From 33fade572760b88b1c537e07514e72e66a49b49e Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 9 Sep 2026 06:23:05 +0300 Subject: [PATCH] feat(cli): print the OAuth authorization URL in daemon-mode auth login POST /api/v1/servers/{name}/login already returns browser_opened and auth_url, but cliclient discarded them, so 'mcpproxy auth login --server=' in daemon mode only printed a generic success line. On a headless host that left the user with no way to finish the flow. - cliclient: add OAuthLoginResult and TriggerOAuthLoginWithResult; the old TriggerOAuthLogin wraps it, so the TUI interface is unchanged. - auth login (daemon branch): print the URL prominently when the browser could not be opened (with the daemon's reason), and as an 'if the browser did not open, visit:' fallback when it did, matching the standalone path. - Tests: httptest decode coverage for both browser states and the API-error path; output assertions for the three CLI branches. Unblocks the diagnostics 'Sign in' fixer from #1218 promising the URL is printed for its headless fallback. --- cmd/mcpproxy/auth_cmd.go | 34 +++++++-- cmd/mcpproxy/auth_cmd_test.go | 47 ++++++++++++ internal/cliclient/client.go | 64 ++++++++++++++--- internal/cliclient/client_tools_test.go | 96 +++++++++++++++++++++++++ 4 files changed, 227 insertions(+), 14 deletions(-) diff --git a/cmd/mcpproxy/auth_cmd.go b/cmd/mcpproxy/auth_cmd.go index 738e7e7a6..00a05c8b0 100644 --- a/cmd/mcpproxy/auth_cmd.go +++ b/cmd/mcpproxy/auth_cmd.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "os" "path/filepath" "strings" @@ -647,7 +648,8 @@ func runAuthLoginClientMode(ctx context.Context, client *cliclient.Client, serve fmt.Fprintf(os.Stderr, "ℹ️ Using daemon mode - coordinating OAuth with running server\n\n") // Trigger OAuth via daemon - if err := client.TriggerOAuthLogin(ctx, serverName); err != nil { + result, err := client.TriggerOAuthLoginWithResult(ctx, serverName) + if err != nil { // Spec 020: Check for structured OAuth errors and display rich output var oauthFlowErr *contracts.OAuthFlowError if errors.As(err, &oauthFlowErr) { @@ -665,13 +667,37 @@ func runAuthLoginClientMode(ctx context.Context, client *cliclient.Client, serve return cliError("failed to trigger OAuth login via daemon", err) } - fmt.Printf("✅ OAuth authentication flow initiated successfully for server: %s\n", serverName) - fmt.Println(" The daemon will handle the OAuth callback and update server state.") - fmt.Println(" Check 'mcpproxy upstream list' to verify authentication status.") + printDaemonOAuthLoginResult(os.Stdout, serverName, result) return nil } +// printDaemonOAuthLoginResult reports the outcome of a daemon-mode login trigger. +// When the daemon could not open a browser (headless host, SSH session, HEADLESS=1) +// the authorization URL is the only way for the user to finish the flow, so it is +// printed prominently; when the browser did open it is still printed as a fallback, +// mirroring the standalone path in internal/upstream/core/connection_oauth.go. +func printDaemonOAuthLoginResult(w io.Writer, serverName string, result *cliclient.OAuthLoginResult) { + fmt.Fprintf(w, "✅ OAuth authentication flow initiated successfully for server: %s\n", serverName) + + if result != nil && result.AuthURL != "" { + if result.BrowserOpened { + fmt.Fprintln(w, " If the browser did not open, visit:") + } else { + fmt.Fprintln(w, "⚠️ Could not open a browser automatically.") + if result.BrowserError != "" { + fmt.Fprintf(w, " Reason: %s\n", result.BrowserError) + } + fmt.Fprintln(w, " Open this URL in a browser to complete authentication:") + } + fmt.Fprintf(w, " %s\n", result.AuthURL) + fmt.Fprintln(w) + } + + fmt.Fprintln(w, " The daemon will handle the OAuth callback and update server state.") + fmt.Fprintln(w, " Check 'mcpproxy upstream list' to verify authentication status.") +} + // runAuthLoginStandalone executes OAuth login in standalone mode (original behavior). func runAuthLoginStandalone(ctx context.Context, serverName string) error { fmt.Printf("🔐 Manual OAuth Authentication - Server: %s\n", serverName) diff --git a/cmd/mcpproxy/auth_cmd_test.go b/cmd/mcpproxy/auth_cmd_test.go index f6a77e68d..0dbce76df 100644 --- a/cmd/mcpproxy/auth_cmd_test.go +++ b/cmd/mcpproxy/auth_cmd_test.go @@ -1,11 +1,13 @@ package main import ( + "bytes" "context" "runtime" "strings" "testing" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/socket" @@ -266,3 +268,48 @@ func TestFilterOAuthServers(t *testing.T) { }) } } + +func TestPrintDaemonOAuthLoginResult_BrowserNotOpened(t *testing.T) { + var out bytes.Buffer + printDaemonOAuthLoginResult(&out, "github", &cliclient.OAuthLoginResult{ + ServerName: "github", + AuthURL: "https://auth.example.com/authorize?state=abc", + BrowserOpened: false, + BrowserError: "HEADLESS mode - browser not opened. Please open the auth_url manually.", + }) + + got := out.String() + assert.Contains(t, got, "OAuth authentication flow initiated successfully for server: github") + assert.Contains(t, got, "Could not open a browser automatically") + assert.Contains(t, got, "HEADLESS mode - browser not opened") + assert.Contains(t, got, "https://auth.example.com/authorize?state=abc") + assert.Contains(t, got, "mcpproxy upstream list") +} + +func TestPrintDaemonOAuthLoginResult_BrowserOpened(t *testing.T) { + var out bytes.Buffer + printDaemonOAuthLoginResult(&out, "github", &cliclient.OAuthLoginResult{ + ServerName: "github", + AuthURL: "https://auth.example.com/authorize?state=abc", + BrowserOpened: true, + }) + + got := out.String() + assert.Contains(t, got, "OAuth authentication flow initiated successfully for server: github") + assert.Contains(t, got, "If the browser did not open, visit:") + assert.Contains(t, got, "https://auth.example.com/authorize?state=abc") + assert.NotContains(t, got, "Could not open a browser automatically") +} + +func TestPrintDaemonOAuthLoginResult_NoAuthURL(t *testing.T) { + var out bytes.Buffer + printDaemonOAuthLoginResult(&out, "github", &cliclient.OAuthLoginResult{ + ServerName: "github", + BrowserOpened: false, + }) + + got := out.String() + assert.Contains(t, got, "OAuth authentication flow initiated successfully for server: github") + assert.NotContains(t, got, "visit:") + assert.Contains(t, got, "mcpproxy upstream list") +} diff --git a/internal/cliclient/client.go b/internal/cliclient/client.go index 0b9677fa3..fc0e8017d 100644 --- a/internal/cliclient/client.go +++ b/internal/cliclient/client.go @@ -1191,25 +1191,47 @@ func (c *Client) SetAllToolsEnabled(ctx context.Context, serverName string, enab return apiResp.Data.Changed, nil } +// OAuthLoginResult carries the fields of the daemon's OAuthStartResponse that the +// CLI needs to guide a user through a headless login: whether the daemon managed to +// open a browser and, if not, the authorization URL to visit manually. +type OAuthLoginResult struct { + ServerName string + CorrelationID string + AuthURL string + BrowserOpened bool + BrowserError string + Message string +} + // TriggerOAuthLogin initiates OAuth authentication flow for a server. // Returns *contracts.OAuthFlowError for structured OAuth errors (Spec 020). +// Use TriggerOAuthLoginWithResult when the caller needs the auth URL / browser status. func (c *Client) TriggerOAuthLogin(ctx context.Context, serverName string) error { + _, err := c.TriggerOAuthLoginWithResult(ctx, serverName) + return err +} + +// TriggerOAuthLoginWithResult initiates the OAuth flow for a server and returns the +// daemon's browser status and authorization URL so the CLI can print the URL when +// the browser could not be opened (headless hosts, SSH sessions). +// Returns *contracts.OAuthFlowError for structured OAuth errors (Spec 020). +func (c *Client) TriggerOAuthLoginWithResult(ctx context.Context, serverName string) (*OAuthLoginResult, error) { url := fmt.Sprintf("%s/api/v1/servers/%s/login", c.baseURL, serverName) req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, nil) if err != nil { - return fmt.Errorf("failed to create request: %w", err) + return nil, fmt.Errorf("failed to create request: %w", err) } c.prepareRequest(ctx, req) resp, err := c.httpClient.Do(req) if err != nil { - return fmt.Errorf("failed to call login API: %w", err) + return nil, fmt.Errorf("failed to call login API: %w", err) } defer resp.Body.Close() bodyBytes, err := io.ReadAll(resp.Body) if err != nil { - return fmt.Errorf("failed to read response: %w", err) + return nil, fmt.Errorf("failed to read response: %w", err) } // Spec 020: Check for structured OAuth errors on 400 responses @@ -1217,44 +1239,66 @@ func (c *Client) TriggerOAuthLogin(ctx context.Context, serverName string) error // Try to parse as OAuthFlowError var oauthFlowErr contracts.OAuthFlowError if err := json.Unmarshal(bodyBytes, &oauthFlowErr); err == nil && oauthFlowErr.ErrorType != "" { - return &oauthFlowErr + return nil, &oauthFlowErr } // Try to parse as OAuthValidationError var oauthValidationErr contracts.OAuthValidationError if err := json.Unmarshal(bodyBytes, &oauthValidationErr); err == nil && oauthValidationErr.ErrorType != "" { - return &oauthValidationErr + return nil, &oauthValidationErr } // Fall back to generic error - return fmt.Errorf("API returned status %d: %s", resp.StatusCode, string(bodyBytes)) + return nil, fmt.Errorf("API returned status %d: %s", resp.StatusCode, string(bodyBytes)) } if resp.StatusCode != http.StatusOK { - return fmt.Errorf("API returned status %d: %s", resp.StatusCode, string(bodyBytes)) + return nil, fmt.Errorf("API returned status %d: %s", resp.StatusCode, string(bodyBytes)) } var apiResp struct { Success bool `json:"success"` Data struct { + // Legacy fields kept for older daemons that returned {server, action, success}. Server string `json:"server"` Action string `json:"action"` Success bool `json:"success"` + // contracts.OAuthStartResponse fields (Spec 020 phase 3). + ServerName string `json:"server_name"` + CorrelationID string `json:"correlation_id"` + AuthURL string `json:"auth_url"` + BrowserOpened bool `json:"browser_opened"` + BrowserError string `json:"browser_error"` + Message string `json:"message"` } `json:"data"` Error string `json:"error"` RequestID string `json:"request_id"` // T023: Capture request_id for error correlation } if err := json.Unmarshal(bodyBytes, &apiResp); err != nil { - return fmt.Errorf("failed to parse response: %w", err) + return nil, fmt.Errorf("failed to parse response: %w", err) } if !apiResp.Success { // T023: Return APIError with request_id for CLI display - return parseAPIError(apiResp.Error, apiResp.RequestID) + return nil, parseAPIError(apiResp.Error, apiResp.RequestID) } - return nil + result := &OAuthLoginResult{ + ServerName: apiResp.Data.ServerName, + CorrelationID: apiResp.Data.CorrelationID, + AuthURL: apiResp.Data.AuthURL, + BrowserOpened: apiResp.Data.BrowserOpened, + BrowserError: apiResp.Data.BrowserError, + Message: apiResp.Data.Message, + } + if result.ServerName == "" { + result.ServerName = apiResp.Data.Server + } + if result.ServerName == "" { + result.ServerName = serverName + } + return result, nil } // TriggerOAuthLogout clears OAuth token and disconnects a server. diff --git a/internal/cliclient/client_tools_test.go b/internal/cliclient/client_tools_test.go index 3cb3ce904..beae46320 100644 --- a/internal/cliclient/client_tools_test.go +++ b/internal/cliclient/client_tools_test.go @@ -132,3 +132,99 @@ func TestClient_TriggerOAuthLogin_NotConfigured(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "does not have OAuth configured") } + +func TestClient_TriggerOAuthLoginWithResult_DecodesBrowserStatus(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "/api/v1/servers/oauth-server/login", r.URL.Path) + assert.Equal(t, "POST", r.Method) + + // Mirrors handleServerLogin: contracts.OAuthStartResponse wrapped in the success envelope. + response := map[string]interface{}{ + "success": true, + "data": map[string]interface{}{ + "success": true, + "server_name": "oauth-server", + "correlation_id": "corr-123", + "auth_url": "https://auth.example.com/authorize?state=abc", + "browser_opened": false, + "browser_error": "HEADLESS mode - browser not opened. Please open the auth_url manually.", + "message": "Could not open browser automatically. Please open this URL manually: https://auth.example.com/authorize?state=abc", + }, + } + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(response) + })) + defer ts.Close() + + client := NewClient(ts.URL, zap.NewNop().Sugar()) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + result, err := client.TriggerOAuthLoginWithResult(ctx, "oauth-server") + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, "oauth-server", result.ServerName) + assert.Equal(t, "corr-123", result.CorrelationID) + assert.Equal(t, "https://auth.example.com/authorize?state=abc", result.AuthURL) + assert.False(t, result.BrowserOpened) + assert.Equal(t, "HEADLESS mode - browser not opened. Please open the auth_url manually.", result.BrowserError) + assert.Contains(t, result.Message, "Please open this URL manually") +} + +func TestClient_TriggerOAuthLoginWithResult_BrowserOpened(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + response := map[string]interface{}{ + "success": true, + "data": map[string]interface{}{ + "success": true, + "server_name": "oauth-server", + "auth_url": "https://auth.example.com/authorize?state=abc", + "browser_opened": true, + "message": "OAuth authentication started", + }, + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(response) + })) + defer ts.Close() + + client := NewClient(ts.URL, zap.NewNop().Sugar()) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + result, err := client.TriggerOAuthLoginWithResult(ctx, "oauth-server") + require.NoError(t, err) + require.NotNil(t, result) + assert.True(t, result.BrowserOpened) + assert.Equal(t, "https://auth.example.com/authorize?state=abc", result.AuthURL) + assert.Empty(t, result.BrowserError) +} + +func TestClient_TriggerOAuthLoginWithResult_APIError(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + response := map[string]interface{}{ + "success": false, + "error": "Server does not have OAuth configured", + "request_id": "req-42", + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(response) + })) + defer ts.Close() + + client := NewClient(ts.URL, zap.NewNop().Sugar()) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + result, err := client.TriggerOAuthLoginWithResult(ctx, "oauth-server") + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "does not have OAuth configured") +}