Skip to content

feat(login): extend --auto-open-browser to work with --web #2115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(login): extend --auto-open-browser to work with --web #2115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions pkg/cli/login/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
`)
Expand Down Expand Up @@ -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
}

Expand All @@ -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
Expand Down Expand Up @@ -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")
Expand Down
10 changes: 8 additions & 2 deletions pkg/cli/login/loginoptions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
139 changes: 139 additions & 0 deletions pkg/cli/login/loginoptions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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) {
Copy link
Member

@ardaguclu ardaguclu Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this test function. Because it seems that it does not exercise the behavior of oc command (it exercises its own custom loginhandler).

Copy link
Author

@celebdor celebdor Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'll do that!

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)
}
})
}
}
Comment on lines +549 to +625
Copy link

@coderabbitai coderabbitai bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Refactor test to verify actual implementation rather than a mock.

This test defines a local loginURLHandler function (lines 587-598) that duplicates the expected implementation logic, then tests that mock function instead of the actual LoginOptions methods. This approach has significant limitations:

  • If the real implementation in loginoptions.go changes, this test won't catch regressions
  • The test validates its own mock logic, not production code
  • Provides false confidence since it doesn't exercise actual code paths

Consider restructuring to:

  1. Call the actual LoginOptions method that handles web login
  2. Mock the browser-opening dependency (e.g., inject a test browser.OpenURL implementation)
  3. Verify behavior through the actual code path rather than reimplementing the logic in the test

Example approach:

-// 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)
+// Mock the browser opener to avoid actually opening a browser
+options.browserOpener = func(url string) error {
+    return nil // no-op in tests
+}
+
+// Call the actual method that handles web login URL
+// (adjust based on actual method name in LoginOptions)
+err := options.handleWebLoginURL("https://example.com/oauth/authorize?test=1")

This ensures the test validates actual production behavior.

Committable suggestion skipped: line range outside the PR's diff.


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) {
Expand Down