From b6d05d7d673982cf93fac156c0fabf3d638043c4 Mon Sep 17 00:00:00 2001 From: Antoni Segura Puimedon Date: Fri, 3 Oct 2025 12:30:41 +0200 Subject: [PATCH] feat(login): extend --auto-open-browser to work with --web MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the existing --auto-open-browser flag to support web login in addition to OIDC login. This provides a consistent interface for controlling browser behavior across both authentication methods. Changes: - Extend --auto-open-browser flag to work with both --web and --exec-plugin - Update flag documentation to clarify default behavior (true for --web, false for OIDC) - Set default to true when using --web (unless explicitly overridden) - Remove --auto-open-browser from OIDC-only validation checks - Update login examples to show --auto-open-browser=false usage - Update test coverage to reflect new behavior When --web is used with --auto-open-browser=false, the authorization URL is printed instead of being automatically opened in a browser. This is useful for environments where browser automation is not desired or possible. Fixes: CNTRLPLANE-1538 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- pkg/cli/login/login.go | 12 ++- pkg/cli/login/loginoptions.go | 10 ++- pkg/cli/login/loginoptions_test.go | 139 +++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 4 deletions(-) diff --git a/pkg/cli/login/login.go b/pkg/cli/login/login.go index 2e04feea30..d61e1a4bb3 100644 --- a/pkg/cli/login/login.go +++ b/pkg/cli/login/login.go @@ -47,6 +47,9 @@ var ( # Log in to the given server through a browser oc login localhost:8443 --web --callback-port 8280 + # Log in to the given server through a browser without opening it automatically (print URL only) + oc login localhost:8443 --web --auto-open-browser=false --callback-port 8280 + # Log in to the external OIDC issuer through Auth Code + PKCE by starting a local server listening on port 8080 oc login localhost:8443 --exec-plugin=oc-oidc --client-id=client-id --extra-scopes=email,profile --callback-port=8080 `) @@ -98,7 +101,7 @@ func NewCmdLogin(f kcmdutil.Factory, streams genericiooptions.IOStreams) *cobra. cmds.Flags().StringSliceVar(&o.OIDCExtraScopes, "extra-scopes", o.OIDCExtraScopes, "Experimental: Extra scopes for external OIDC issuer. Optional.") cmds.Flags().StringVar(&o.OIDCIssuerURL, "issuer-url", o.OIDCIssuerURL, "Experimental: Issuer url for external issuer. Required.") cmds.Flags().StringVar(&o.OIDCCAFile, "oidc-certificate-authority", o.OIDCCAFile, "Experimental: The path to a certificate authority bundle to use when communicating with external OIDC issuer.") - cmds.Flags().BoolVar(&o.OIDCAutoOpenBrowser, "auto-open-browser", o.OIDCAutoOpenBrowser, "Experimental: Specify browser is automatically opened or not for external OIDC issuer. Disabled by default.") + cmds.Flags().BoolVar(&o.OIDCAutoOpenBrowser, "auto-open-browser", o.OIDCAutoOpenBrowser, "Experimental: Automatically open browser for login. When used with --web, defaults to true. When used with --exec-plugin for external OIDC, defaults to false.") return cmds } @@ -120,6 +123,11 @@ func (o *LoginOptions) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []s } o.RequestTimeout = timeout + // Set default value for auto-open-browser when using web login + if o.WebLogin && !cmd.Flags().Changed("auto-open-browser") { + o.OIDCAutoOpenBrowser = true + } + parsedDefaultClusterURL, err := url.Parse(defaultClusterURL) if err != nil { return err @@ -191,7 +199,7 @@ func (o LoginOptions) Validate(cmd *cobra.Command, serverFlag string, args []str return errors.New("currently only oc-oidc is supported") } - oidcOptionsSet := o.OIDCClientID != "" || o.OIDCClientSecret != "" || len(o.OIDCExtraScopes) > 0 || o.OIDCIssuerURL != "" || o.OIDCAutoOpenBrowser + oidcOptionsSet := o.OIDCClientID != "" || o.OIDCClientSecret != "" || len(o.OIDCExtraScopes) > 0 || o.OIDCIssuerURL != "" if o.OIDCExecPluginType == "" && oidcOptionsSet { return errors.New("please specify --exec-plugin type. Currently only oc-oidc is supported") diff --git a/pkg/cli/login/loginoptions.go b/pkg/cli/login/loginoptions.go index efbe6b57fe..62a998036d 100644 --- a/pkg/cli/login/loginoptions.go +++ b/pkg/cli/login/loginoptions.go @@ -317,8 +317,14 @@ func (o *LoginOptions) gatherAuthInfo() error { if o.WebLogin { loginURLHandler := func(u *url.URL) error { loginURL := u.String() - fmt.Fprintf(o.Out, "Opening login URL in the default browser: %s\n", loginURL) - return browser.OpenURL(loginURL) + if !o.OIDCAutoOpenBrowser { + fmt.Fprintf(o.Out, "Please visit the following URL in your browser: %s\n", loginURL) + fmt.Fprintf(o.Out, "The callback server is listening and will receive the authentication response.\n") + return nil + } else { + fmt.Fprintf(o.Out, "Opening login URL in the default browser: %s\n", loginURL) + return browser.OpenURL(loginURL) + } } token, err = tokenrequest.RequestTokenWithLocalCallback(o.Config, loginURLHandler, int(o.CallbackPort)) } else { diff --git a/pkg/cli/login/loginoptions_test.go b/pkg/cli/login/loginoptions_test.go index dfd61899e0..fcf084b9a2 100644 --- a/pkg/cli/login/loginoptions_test.go +++ b/pkg/cli/login/loginoptions_test.go @@ -1,13 +1,16 @@ package login import ( + "bytes" "crypto/tls" "encoding/json" "encoding/pem" "fmt" "net/http" "net/http/httptest" + "net/url" "regexp" + "strings" "testing" "github.com/MakeNowJust/heredoc" @@ -485,6 +488,142 @@ func TestPreserveExecProviderOnUsernameLogin(t *testing.T) { } } +func TestValidateAutoOpenBrowser(t *testing.T) { + testCases := []struct { + name string + webLogin bool + autoOpenBrowser bool + expectedError string + }{ + { + name: "valid: --web with --auto-open-browser", + webLogin: true, + autoOpenBrowser: true, + expectedError: "", + }, + { + name: "valid: --web with --auto-open-browser=false", + webLogin: true, + autoOpenBrowser: false, + expectedError: "", + }, + { + name: "valid: --web without --auto-open-browser (will default to true)", + webLogin: true, + autoOpenBrowser: false, + expectedError: "", + }, + { + name: "valid: neither --web nor --auto-open-browser", + webLogin: false, + autoOpenBrowser: false, + expectedError: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + options := &LoginOptions{ + Server: "https://api.test.devcluster.openshift.com:6443", // Consistent with existing OpenShift tests + WebLogin: tc.webLogin, + OIDCAutoOpenBrowser: tc.autoOpenBrowser, + StartingKubeConfig: &kclientcmdapi.Config{}, + } + + err := options.Validate(nil, "", []string{}) + if tc.expectedError == "" { + if err != nil { + t.Errorf("expected no error, but got: %v", err) + } + } else { + if err == nil { + t.Errorf("expected error '%s', but got no error", tc.expectedError) + } else if err.Error() != tc.expectedError { + t.Errorf("expected error '%s', but got: %v", tc.expectedError, err) + } + } + }) + } +} + +func TestAutoOpenBrowserURLHandling(t *testing.T) { + testCases := []struct { + name string + autoOpenBrowser bool + expectedOutputRegex string + shouldNotContain string + }{ + { + name: "with --auto-open-browser=false prints URL without opening", + autoOpenBrowser: false, + expectedOutputRegex: "Please visit the following URL in your browser: https://example.com/oauth/authorize\\?test=1\nThe callback server is listening and will receive the authentication response.\n", + shouldNotContain: "Opening login URL in the default browser", + }, + { + name: "with --auto-open-browser=true shows opening message", + autoOpenBrowser: true, + expectedOutputRegex: "Opening login URL in the default browser: https://example.com/oauth/authorize\\?test=1\n", + shouldNotContain: "Please visit the following URL", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Capture output + outBuf := &bytes.Buffer{} + streams := genericiooptions.IOStreams{ + In: strings.NewReader(""), + Out: outBuf, + ErrOut: &bytes.Buffer{}, + } + + options := &LoginOptions{ + WebLogin: true, + OIDCAutoOpenBrowser: tc.autoOpenBrowser, + IOStreams: streams, + } + + // Create a test login URL handler that matches the actual implementation + loginURLHandler := func(u *url.URL) error { + loginURL := u.String() + if !options.OIDCAutoOpenBrowser { + fmt.Fprintf(options.Out, "Please visit the following URL in your browser: %s\n", loginURL) + fmt.Fprintf(options.Out, "The callback server is listening and will receive the authentication response.\n") + return nil + } else { + fmt.Fprintf(options.Out, "Opening login URL in the default browser: %s\n", loginURL) + // Don't actually call browser.OpenURL in tests + return nil + } + } + + // Test the handler with a test URL + testURL, _ := url.Parse("https://example.com/oauth/authorize?test=1") + err := loginURLHandler(testURL) + + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + output := outBuf.String() + + // Check expected output using regex + matched, regexErr := regexp.MatchString(tc.expectedOutputRegex, output) + if regexErr != nil { + t.Fatalf("regex error: %v", regexErr) + } + if !matched { + t.Errorf("output did not match expected pattern.\nExpected pattern: %s\nActual output: %s", tc.expectedOutputRegex, output) + } + + // Check that certain strings are not present + if tc.shouldNotContain != "" && strings.Contains(output, tc.shouldNotContain) { + t.Errorf("output should not contain '%s', but got: %s", tc.shouldNotContain, output) + } + }) + } +} + func newTLSServer(certString, keyString string) (*httptest.Server, error) { invoked := make(chan struct{}, 1) server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {