diff --git a/internal/runtime/oauth_health.go b/internal/runtime/oauth_health.go new file mode 100644 index 000000000..f3a81b05d --- /dev/null +++ b/internal/runtime/oauth_health.go @@ -0,0 +1,162 @@ +package runtime + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "time" + + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" +) + +// OAuthHealthFields is the OAuth token state a health.HealthCalculatorInput +// needs. Every surface that calls health.CalculateHealth (REST/tray via +// GetAllServers, the MCP upstream_servers list) derives it here so an empty +// status is never mistaken for "no token". +type OAuthHealthFields struct { + // Config is the serialized OAuth config for the server map; nil when the + // server does not use OAuth. OAuthRequired == (Config != nil). + Config map[string]interface{} + Authenticated bool + Status string // "authenticated", "expired", "error", "none" or "" + TokenExpiresAt time.Time + HasRefreshToken bool + CallTimeOAuthRequired bool +} + +// OAuthHealthState derives the OAuth token state for the named server from the +// persisted token store, the config and the last upstream error. +// rawLastError is the uncollapsed last error. +func (r *Runtime) OAuthHealthState(name string, cfg *config.ServerConfig, rawLastError string) OAuthHealthFields { + var f OAuthHealthFields + var ( + oauthConfig map[string]interface{} + authenticated bool + oauthStatus string + tokenExpiresAt time.Time + hasRefreshToken bool + ) + var url string + if cfg != nil { + url = cfg.URL + // Serialize OAuth config if present (explicit config) + if cfg.OAuth != nil { + oauthConfig = map[string]interface{}{ + "client_id": cfg.OAuth.ClientID, + "scopes": cfg.OAuth.Scopes, + "extra_params": cfg.OAuth.ExtraParams, + "pkce_enabled": cfg.OAuth.PKCEEnabled, + // auth_url, token_url will be populated from OAuth runtime state if available + "auth_url": "", + "token_url": "", + } + } + + // GH #1172: a token record left behind by an earlier OAuth login + // is not evidence about a server that now authenticates with a + // static Authorization header — see StoredOAuthTokenInPlay. + tokenInPlay := r.StoredOAuthTokenInPlay(name, cfg) + + // Check if server has valid OAuth token in storage + // IMPORTANT: This runs for ALL servers with a URL, including autodiscovery servers + // PersistentTokenStore uses serverKey (name + URL hash), not just server name + // We need to generate the same key format: "servername_hash16" + if url != "" && r.storageManager != nil && tokenInPlay { + r.logger.Debug("Checking OAuth token in storage", + zap.String("server", name), + // #1158: the configured upstream URL routinely carries a + // `?token=` credential; the host and path stay readable. + zap.String("url", oauth.AuditRedaction.URLValue(url)), + zap.Bool("has_explicit_oauth_config", cfg.OAuth != nil)) + + // Generate server key matching PersistentTokenStore format + combined := fmt.Sprintf("%s|%s", name, url) + hash := sha256.Sum256([]byte(combined)) + hashStr := hex.EncodeToString(hash[:]) + serverKey := fmt.Sprintf("%s_%s", name, hashStr[:16]) + + r.logger.Debug("Generated OAuth token lookup key", + zap.String("server", name), + zap.String("server_key", serverKey)) + + token, err := r.storageManager.GetOAuthToken(serverKey) + r.logger.Debug("OAuth token lookup result", + zap.String("server", name), + zap.String("server_key", serverKey), + zap.Bool("token_nil", token == nil), + zap.Error(err)) + + if err == nil && token != nil { + authenticated = true + tokenExpiresAt = token.ExpiresAt + hasRefreshToken = token.RefreshToken != "" + r.logger.Info("OAuth token found for server", + zap.String("server", name), + zap.String("server_key", serverKey), + zap.Time("expires_at", token.ExpiresAt), + zap.Bool("has_refresh_token", hasRefreshToken)) + + // For autodiscovery servers (no explicit OAuth config), create minimal oauthConfig + if oauthConfig == nil { + oauthConfig = map[string]interface{}{ + "autodiscovery": true, + } + } + + // Add token expiration info to oauth config + if !token.ExpiresAt.IsZero() { + oauthConfig["token_expires_at"] = token.ExpiresAt.Format(time.RFC3339) + // Check if token is expired + isValid := time.Now().Before(token.ExpiresAt) + oauthConfig["token_valid"] = isValid + if isValid { + oauthStatus = string(oauth.OAuthStatusAuthenticated) + } else { + oauthStatus = string(oauth.OAuthStatusExpired) + } + } else { + // No expiration means token is valid indefinitely + oauthConfig["token_valid"] = true + oauthStatus = string(oauth.OAuthStatusAuthenticated) + } + } else { + // No token found - check if OAuth config exists to determine status + if oauthConfig != nil { + oauthStatus = string(oauth.OAuthStatusNone) + } + } + } + } + + // Check for OAuth error in last_error - this indicates OAuth autodiscovery detected + // an OAuth-required server that has no token (user needs to authenticate) + if oauthStatus != string(oauth.OAuthStatusExpired) && rawLastError != "" { + if oauth.IsOAuthError(rawLastError) { + // If we have no oauthConfig yet, this is an autodiscovery server that needs OAuth + if oauthConfig == nil { + oauthConfig = map[string]interface{}{ + "autodiscovery": true, + } + // Set status to "none" - user hasn't authenticated yet + oauthStatus = string(oauth.OAuthStatusNone) + } else { + // Has config but error - token might be invalid + oauthStatus = string(oauth.OAuthStatusError) + } + } + } + if r.upstreamManager != nil { + if client, exists := r.upstreamManager.GetClient(name); exists && client != nil { + f.CallTimeOAuthRequired = client.IsOAuthCallRequired() + } + } + f.Config = oauthConfig + f.Authenticated = authenticated + f.Status = oauthStatus + f.TokenExpiresAt = tokenExpiresAt + f.HasRefreshToken = hasRefreshToken + return f +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 29ea04384..494282912 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -2,8 +2,6 @@ package runtime import ( "context" - "crypto/sha256" - "encoding/hex" "encoding/json" "errors" "fmt" @@ -2538,123 +2536,20 @@ func (r *Runtime) GetAllServers() ([]map[string]interface{}, error) { // Extract created time and config fields var created time.Time var url, command, protocol string - var oauthConfig map[string]interface{} - var authenticated bool - var oauthStatus string // OAuth status: "authenticated", "expired", "error", "none" - var tokenExpiresAt time.Time - var hasRefreshToken bool if serverStatus.Config != nil { created = serverStatus.Config.Created url = serverStatus.Config.URL command = serverStatus.Config.Command protocol = serverStatus.Config.Protocol - - // Serialize OAuth config if present (explicit config) - if serverStatus.Config.OAuth != nil { - oauthConfig = map[string]interface{}{ - "client_id": serverStatus.Config.OAuth.ClientID, - "scopes": serverStatus.Config.OAuth.Scopes, - "extra_params": serverStatus.Config.OAuth.ExtraParams, - "pkce_enabled": serverStatus.Config.OAuth.PKCEEnabled, - // auth_url, token_url will be populated from OAuth runtime state if available - "auth_url": "", - "token_url": "", - } - } - - // GH #1172: a token record left behind by an earlier OAuth login - // is not evidence about a server that now authenticates with a - // static Authorization header — see StoredOAuthTokenInPlay. - tokenInPlay := r.StoredOAuthTokenInPlay(serverStatus.Name, serverStatus.Config) - - // Check if server has valid OAuth token in storage - // IMPORTANT: This runs for ALL servers with a URL, including autodiscovery servers - // PersistentTokenStore uses serverKey (name + URL hash), not just server name - // We need to generate the same key format: "servername_hash16" - if url != "" && r.storageManager != nil && tokenInPlay { - r.logger.Debug("Checking OAuth token in storage", - zap.String("server", serverStatus.Name), - // #1158: the configured upstream URL routinely carries a - // `?token=` credential; the host and path stay readable. - zap.String("url", oauth.AuditRedaction.URLValue(url)), - zap.Bool("has_explicit_oauth_config", serverStatus.Config.OAuth != nil)) - - // Generate server key matching PersistentTokenStore format - combined := fmt.Sprintf("%s|%s", serverStatus.Name, url) - hash := sha256.Sum256([]byte(combined)) - hashStr := hex.EncodeToString(hash[:]) - serverKey := fmt.Sprintf("%s_%s", serverStatus.Name, hashStr[:16]) - - r.logger.Debug("Generated OAuth token lookup key", - zap.String("server", serverStatus.Name), - zap.String("server_key", serverKey)) - - token, err := r.storageManager.GetOAuthToken(serverKey) - r.logger.Debug("OAuth token lookup result", - zap.String("server", serverStatus.Name), - zap.String("server_key", serverKey), - zap.Bool("token_nil", token == nil), - zap.Error(err)) - - if err == nil && token != nil { - authenticated = true - tokenExpiresAt = token.ExpiresAt - hasRefreshToken = token.RefreshToken != "" - r.logger.Info("OAuth token found for server", - zap.String("server", serverStatus.Name), - zap.String("server_key", serverKey), - zap.Time("expires_at", token.ExpiresAt), - zap.Bool("has_refresh_token", hasRefreshToken)) - - // For autodiscovery servers (no explicit OAuth config), create minimal oauthConfig - if oauthConfig == nil { - oauthConfig = map[string]interface{}{ - "autodiscovery": true, - } - } - - // Add token expiration info to oauth config - if !token.ExpiresAt.IsZero() { - oauthConfig["token_expires_at"] = token.ExpiresAt.Format(time.RFC3339) - // Check if token is expired - isValid := time.Now().Before(token.ExpiresAt) - oauthConfig["token_valid"] = isValid - if isValid { - oauthStatus = string(oauth.OAuthStatusAuthenticated) - } else { - oauthStatus = string(oauth.OAuthStatusExpired) - } - } else { - // No expiration means token is valid indefinitely - oauthConfig["token_valid"] = true - oauthStatus = string(oauth.OAuthStatusAuthenticated) - } - } else { - // No token found - check if OAuth config exists to determine status - if oauthConfig != nil { - oauthStatus = string(oauth.OAuthStatusNone) - } - } - } - } - - // Check for OAuth error in last_error - this indicates OAuth autodiscovery detected - // an OAuth-required server that has no token (user needs to authenticate) - if oauthStatus != string(oauth.OAuthStatusExpired) && serverStatus.LastError != "" { - if oauth.IsOAuthError(serverStatus.LastError) { - // If we have no oauthConfig yet, this is an autodiscovery server that needs OAuth - if oauthConfig == nil { - oauthConfig = map[string]interface{}{ - "autodiscovery": true, - } - // Set status to "none" - user hasn't authenticated yet - oauthStatus = string(oauth.OAuthStatusNone) - } else { - // Has config but error - token might be invalid - oauthStatus = string(oauth.OAuthStatusError) - } - } } + // One derivation of the OAuth token state, shared with the MCP + // upstream_servers list so every surface reports the same health. + oauthState := r.OAuthHealthState(serverStatus.Name, serverStatus.Config, serverStatus.LastError) + oauthConfig := oauthState.Config + authenticated := oauthState.Authenticated + oauthStatus := oauthState.Status + tokenExpiresAt := oauthState.TokenExpiresAt + hasRefreshToken := oauthState.HasRefreshToken // Audit F12: mcp-go's streamable-HTTP transport wraps a send failure with // the same "failed to send request" text at two nesting levels, so the diff --git a/internal/server/list_upstreams_oauth_health_test.go b/internal/server/list_upstreams_oauth_health_test.go new file mode 100644 index 000000000..3a5490fae --- /dev/null +++ b/internal/server/list_upstreams_oauth_health_test.go @@ -0,0 +1,128 @@ +package server + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime/stateview" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" +) + +// #1522: the MCP upstream_servers list built its own health input and never +// supplied the OAuth token state, so a server with a valid stored token was +// reported "Authentication required" there while REST/tray said healthy. +func TestUpstreamServersList_OAuthHealthMatchesRuntime(t *testing.T) { + week := 7 * 24 * time.Hour + cases := []struct { + name string + expiresIn time.Duration // 0 => no stored token + refresh string + disabled bool + quarantined bool + wantHealthy bool + wantNotLogin bool + }{ + {name: "valid_token_48h", expiresIn: 48 * time.Hour, wantHealthy: true, wantNotLogin: true}, + {name: "valid_token_no_expiry_info_week", expiresIn: week, wantHealthy: true, wantNotLogin: true}, + {name: "no_token"}, + {name: "expired_no_refresh", expiresIn: -time.Hour}, + {name: "disabled", expiresIn: 48 * time.Hour, disabled: true}, + {name: "quarantined", expiresIn: 48 * time.Hour, quarantined: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + proxy, rt := createTestProxyWithRuntimeCfg(t, nil, nil) + up := startCountingUpstream(t, proxy, rt, "gh", readSpec("list_issues")) + + sc := &config.ServerConfig{ + Name: "gh", URL: up.URL, Protocol: "streamable-http", + Enabled: !tc.disabled, Quarantined: tc.quarantined, OAuth: &config.OAuthConfig{}, + } + require.NoError(t, proxy.storage.SaveUpstreamServer(sc)) + rt.Supervisor().StateView().UpdateServer("gh", func(s *stateview.ServerStatus) { + s.Config = sc + s.Enabled = sc.Enabled + s.Quarantined = sc.Quarantined + s.State = "connected" + s.Connected = true + }) + if tc.expiresIn != 0 { + require.NoError(t, rt.StorageManager().GetBoltDB().SaveOAuthToken(&storage.OAuthTokenRecord{ + ServerName: oauth.GenerateServerKey("gh", up.URL), DisplayName: "gh", + AccessToken: "tok", TokenType: "Bearer", RefreshToken: tc.refresh, + ExpiresAt: time.Now().Add(tc.expiresIn), Created: time.Now(), Updated: time.Now(), + })) + } + + want := runtimeHealth(t, rt, "gh") + got := listHealth(t, proxy, "gh") + require.Equal(t, want.Level, got.Level) + require.Equal(t, want.AdminState, got.AdminState) + require.Equal(t, want.Summary, got.Summary) + require.Equal(t, want.Action, got.Action) + if tc.wantHealthy { + require.Equal(t, "healthy", got.Level) + } + if tc.wantNotLogin { + require.NotEqual(t, "login", got.Action) + } + if !tc.wantHealthy && !tc.disabled && !tc.quarantined { + require.NotEqual(t, "healthy", got.Level) + } + if tc.disabled { + require.Equal(t, "disabled", got.AdminState) + } + if tc.quarantined { + require.Equal(t, "quarantined", got.AdminState) + } + }) + } +} + +func runtimeHealth(t *testing.T, rt *runtime.Runtime, name string) *contracts.HealthStatus { + t.Helper() + servers, err := rt.GetAllServers() + require.NoError(t, err) + for _, s := range servers { + if s["name"] == name { + hs, ok := s["health"].(*contracts.HealthStatus) + require.True(t, ok, "health type %T", s["health"]) + return hs + } + } + t.Fatalf("server %q not in runtime projection", name) + return nil +} + +func listHealth(t *testing.T, proxy *MCPProxyServer, name string) *contracts.HealthStatus { + t.Helper() + res, err := proxy.handleUpstreamServers(context.Background(), mcp.CallToolRequest{ + Params: mcp.CallToolParams{Arguments: map[string]interface{}{"operation": "list"}}, + }) + require.NoError(t, err) + var payload struct { + Servers []struct { + Name string `json:"name"` + Health *contracts.HealthStatus `json:"health"` + } `json:"servers"` + } + require.NoError(t, json.Unmarshal([]byte(resultText(t, res)), &payload)) + for _, s := range payload.Servers { + if s.Name == name { + require.NotNil(t, s.Health) + return s.Health + } + } + t.Fatalf("server %q not in MCP list: %s", name, resultText(t, res)) + return nil +} diff --git a/internal/server/mcp.go b/internal/server/mcp.go index b43048f14..627960bf5 100644 --- a/internal/server/mcp.go +++ b/internal/server/mcp.go @@ -4861,6 +4861,16 @@ func (p *MCPProxyServer) handleListUpstreams(ctx context.Context) (*mcp.CallTool // Read through the runtime seam so a stale schedule for a server that // no longer uses OAuth is not reported here either (GH #1172). if p.mainServer != nil && p.mainServer.runtime != nil { + // #1522: OAuth token state from the runtime's single derivation; + // an empty OAuthStatus reads as "no token" (sign-in required). + oauthState := p.mainServer.runtime.OAuthHealthState(server.Name, server, lastError) + healthInput.OAuthRequired = oauthState.Config != nil + healthInput.OAuthStatus = oauthState.Status + healthInput.HasRefreshToken = oauthState.HasRefreshToken + healthInput.CallTimeOAuthRequired = oauthState.CallTimeOAuthRequired + if !oauthState.TokenExpiresAt.IsZero() { + healthInput.TokenExpiresAt = &oauthState.TokenExpiresAt + } if refreshState := p.mainServer.runtime.HealthRefreshState(server.Name, server); refreshState != nil { healthInput.RefreshState = health.RefreshState(refreshState.State) healthInput.RefreshRetryCount = refreshState.RetryCount diff --git a/internal/server/server.go b/internal/server/server.go index aed53f67b..2c17bffa3 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -1700,9 +1700,16 @@ func (s *Server) GetAllServers() ([]map[string]interface{}, error) { RetryCount: serverStatus.RetryCount, } - // Check if OAuth is required for this server - if serverStatus.Config != nil && serverStatus.Config.OAuth != nil { - healthInput.OAuthRequired = true + // OAuth token state comes from the runtime's single derivation; leaving + // OAuthStatus empty reads as "no token" and reports a signed-in server + // as needing sign-in (#1522). + oauthState := s.runtime.OAuthHealthState(serverStatus.Name, cfg, serverStatus.LastError) + healthInput.OAuthRequired = oauthState.Config != nil + healthInput.OAuthStatus = oauthState.Status + healthInput.HasRefreshToken = oauthState.HasRefreshToken + healthInput.CallTimeOAuthRequired = oauthState.CallTimeOAuthRequired + if !oauthState.TokenExpiresAt.IsZero() { + healthInput.TokenExpiresAt = &oauthState.TokenExpiresAt } // T032: Wire refresh state into health calculation (Spec 023).