fix: skip browser OAuth flow when valid token exists in storage#181
Merged
Conversation
Dumbris
force-pushed
the
fix/oauth-reconnection-after-login
branch
from
beaafac to
243872e
Compare
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 20019441838 --repo smart-mcp-proxy/mcpproxy-go
|
When OAuth was completed via CLI or another process, the daemon would still trigger browser OAuth flow even though a valid token existed in persistent storage. This caused unnecessary OAuth prompts and confusion. Root cause: - `HasRecentOAuthCompletion()` only checks in-memory state (per-process) - Cross-process OAuth completion was not detected - Browser flow was triggered even when valid token existed Fix: - Add direct check for valid persisted token before OAuth flow - Set `skipBrowserFlow` flag when valid token found - Skip browser OAuth and return retriable error when flag is set - Applied to both HTTP and SSE OAuth flows - Applied to both initial connection and MCP init OAuth errors Test results: - Core unit tests pass - OAuth unit tests pass - OAuth E2E tests pass (29 tests) Closes: docs/bugs/oauth-reconnection-after-login.md 🤖 Generated with [Claude Code](https://claude.ai/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add NotifyServerConnecting helper method - Add StateConnecting case to StateChangeNotifier - UI now shows "Connecting" badge immediately when reconnection starts - Triggers SSE event for real-time UI updates 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Previously, ClearOAuthToken() required serverConfig.OAuth != nil, which failed for servers using discovered OAuth (server-announced OAuth). This caused a 500 error when clicking "Logout" in the Web UI for servers like Sentry MCP that use OAuth discovery. The fix removes the OAuth config requirement check, allowing logout to work for both explicit OAuth config and discovered OAuth servers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
If a server is authenticated, only show Logout button. Login button should only appear for non-authenticated servers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The backend now returns a 'connecting' field in the server API response, enabling the Web UI and CLI to show "Connecting" status when a server is in the process of connecting (e.g., during OAuth flow or reconnection). Changes: - contracts/types.go: Add Connecting bool field to Server struct - contracts/converters.go: Extract connecting field in converter - management/service.go: Extract connecting field in ListServers 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The proactive token refresh was failing because RefreshManager was using the storage key format (serverName_hash) instead of the actual server name when looking up servers for refresh. Changes: - Add DisplayName field to OAuthTokenRecord to store actual server name - Add GetServerName() helper that returns DisplayName if set, falls back to ServerName for backward compatibility - Update PersistentTokenStore to store both serverKey and serverName - Update RefreshManager to use GetServerName() when loading tokens This fixes the "server not found" error during proactive token refresh. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add optimistic update to triggerOAuthLogout to immediately set authenticated = false, ensuring the Login button appears right away instead of waiting for SSE event refresh. The issue was that clicking Logout would clear the token but the UI wouldn't update immediately because it was waiting for the servers.changed SSE event to trigger a refresh. Now the authenticated field is cleared optimistically with proper rollback on error. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use oauth_status field for more accurate OAuth state representation: - Frontend: Update needsOAuth/canLogout logic to use oauth_status - Login shows when: disconnected + token expired/error/none - Logout shows when: connected OR has error with valid token - Hidden states: Login during connecting, Logout when token expired - Backend: Add oauth_status and token_expires_at to server response - oauth_status: "authenticated" | "expired" | "error" | "none" - Calculated from stored token validity in runtime.go - CLI: Update auth status to show clearer messages - "Reconnecting (Token Valid)" instead of "Authenticated (Disconnected)" - "Token Expired (Login Required)" for expired tokens - API: Add connecting field to OpenAPI Server schema 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When a user clicks Logout, the server should stay disconnected until the user explicitly logs in again. Previously, the exponential backoff reconnection logic would auto-reconnect the server shortly after logout. This fix introduces a `userLoggedOut` flag in the StateManager that: - Gets set to true when TriggerOAuthLogout is called (before other operations) - Prevents ShouldRetry() from returning true while the flag is set - Gets cleared on StateReady transition (successful connection) - Gets cleared when TriggerOAuthLogin is called (explicit login) The order of operations in TriggerOAuthLogout is critical to prevent race conditions: the flag must be set FIRST before ClearOAuthToken or DisconnectServer, otherwise reconnection could trigger before the flag is set. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The previous fix (7aaba6a) added the userLoggedOut flag to StateManager and prevented ShouldRetry() from returning true after explicit logout. However, the supervisor's reconcile loop was operating at a higher level and still reconnecting servers by computing ActionConnect for any enabled server that wasn't connected. This fix adds IsUserLoggedOut() to the UpstreamInterface and updates computeReconcilePlan() to check this flag before deciding to reconnect. Now when a user clicks Logout: 1. StateManager.userLoggedOut flag is set (prevents retry logic) 2. Supervisor sees the server is disconnected but checks IsUserLoggedOut() 3. If true, supervisor sets ActionNone instead of ActionConnect 4. Server stays disconnected until user explicitly logs in again 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add user_logged_out field to API response and contracts for tracking explicit user logout action - Update supervisor to respect userLoggedOut flag and prevent auto-reconnection after explicit logout - Fix ServerCard.vue button visibility to prevent Login and Logout buttons from appearing simultaneously when OAuth error present with stored token - Update CLI auth status to show disabled/logged-out state correctly - Add OAuth manager tests for token persistence and logout flow - Update OpenAPI spec with user_logged_out field documentation The user_logged_out flag captures user intent to logout separately from connection state and config state, enabling proper UX where: - After logout: Login button visible, no auto-reconnection - Connected: Logout button visible - OAuth error with expired token: Login button visible (not Logout) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add 'connecting' field to get_servers.json golden file to match the actual API response. The 'user_logged_out' field is not included because it uses omitempty and defaults to false. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Dumbris
force-pushed
the
fix/oauth-reconnection-after-login
branch
from
3c54c07 to
895505d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
After completing OAuth via CLI or tray, the daemon would still trigger browser OAuth flow because:
HasRecentOAuthCompletion()only checks in-memory state (per-process)Solution
Test plan
Documentation
Updated
docs/bugs/oauth-reconnection-after-login.mdwith fix details and marked as FIXED.🤖 Generated with Claude Code