Skip to content

feat: support to disable auto open browser#365

Open
patrickmen wants to merge 1 commit into
DingTalk-Real-AI:mainfrom
patrickmen:feat/no-browser
Open

feat: support to disable auto open browser#365
patrickmen wants to merge 1 commit into
DingTalk-Real-AI:mainfrom
patrickmen:feat/no-browser

Conversation

@patrickmen
Copy link
Copy Markdown

@patrickmen patrickmen commented May 27, 2026

Problem

_ = cmd.Flags().MarkHidden("no-browser")

The --no-browser flag was already defined on dws auth login but never wired to the device flow logic — the browser always opened regardless.

Fix

  • Pass the --no-browser flag value into DeviceFlowProvider.NoBrowser and gate the openBrowser call on it.
  • The flag was already defined but never connected — browser always opened regardless of the flag.

Copy link
Copy Markdown

@Bububuger Bububuger left a comment

Choose a reason for hiding this comment

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

LGTM — clean fix wiring the existing --no-browser flag into device flow logic.

@patrickmen patrickmen requested a review from Bububuger June 1, 2026 06:59
@patrickmen
Copy link
Copy Markdown
Author

@PeterGuy326 Could you help review this PR? Thanks a lot!

@PeterGuy326
Copy link
Copy Markdown
Collaborator

Thanks for the fix! The wiring into the device flow is correct: dfPrintDeviceCodeBox already prints the device code and verification URL, so gating openBrowser behind !p.NoBrowser still leaves the user a usable manual path. Low risk there.

One thing to address before merge, though.

The flag is now public, but only works for one of three login paths

dws auth login has three paths, and the browser is opened from two different places:

Path Entry Opens browser at Honors --no-browser?
--token xxx direct token (never opens) n/a
--device device flow internal/auth/device_flow.go ✅ this PR
default (no flags) loopback OAuth scan internal/auth/oauth_provider.go (openBrowser(authURL)) not wired

This PR also un-hides the flag (removes MarkHidden("no-browser")), so it now shows up in --help as a general option. But the default path (the most common dws auth login, loopback OAuth) still calls openBrowser(authURL) unconditionally. So dws auth login --no-browser (no --device) will still open the browser — which is more confusing than the original "flag does nothing", because the flag now looks like a supported, general-purpose option.

Note --no-browser is still meaningful for the loopback flow: the callback needs the browser to hit 127.0.0.1, but suppressing auto-open and printing the URL for the user to open manually is a legitimate use case.

Suggested change

1. Wire NoBrowser into OAuthProvider too. Add a NoBrowser bool field to OAuthProvider and gate the open:

// internal/auth/oauth_provider.go
if !p.NoBrowser {
    if err := openBrowser(authURL); err != nil && p.logger != nil {
        p.logger.Warn(i18n.T("无法自动打开浏览器"), "error", err)
    }
}

And in internal/app/auth_command.go, set it in the default: branch the same way you did for device:

provider := authpkg.NewOAuthProvider(configDir, nil)
provider.Output = cmd.ErrOrStderr()
provider.NoBrowser, _ = cmd.Flags().GetBool("no-browser")

2. (consistency, optional) Force and Device flow through authLoginConfig + resolveAuthLoginConfig, while NoBrowser is read inline with cmd.Flags().GetBool. Consider adding NoBrowser bool to authLoginConfig so it matches the surrounding style.

3. Add a test. The PAT path already has an openBrowserFunc hook used in tests; please add coverage asserting the browser is not opened when NoBrowser is set, for both the device flow and the loopback flow.

Alternative if you'd rather keep the scope small: keep the flag hidden (don't remove MarkHidden), or change the help text to say it only applies to --device. But wiring both paths is the cleaner outcome.

Verification — please attach a screenshot

After the change, please paste a screenshot (or terminal capture) showing each case so we can confirm the loop is closed:

  1. dws auth login --no-browser (default loopback flow) — browser does not open, the auth URL is printed instead.
  2. dws auth login --device --no-browser — browser does not open, device code box is printed.
  3. dws auth login and dws auth login --device (no flag) — browser does open (regression check).

Plus dws auth login --help showing --no-browser is now listed.

@PeterGuy326
Copy link
Copy Markdown
Collaborator

Thanks for the fix — verified locally that the device flow path now works: dws auth login --help shows --no-browser, and dws auth login --device --no-browser no longer opens the browser. (Also approved the workflow run; CI is green.)

One gap before merge: --no-browser only takes effect on the device flow (--device). The default dws auth login goes through the OAuth flow (internal/auth/oauth_provider.go), whose openBrowser(authURL) is not gated — so dws auth login --no-browser (without --device) still opens the browser.

Repro: dws auth login --no-browser → still opens login.dingtalk.com/oauth2/auth?... despite the flag.

The OAuth flow already prints the manual URL ("如果浏览器未自动打开,请手动访问: "), so suppressing the auto-open is safe — the user just opens that link. Suggested follow-up (3 small changes), mirroring what you did for the device flow:

  1. internal/auth/oauth_provider.go — add a field to OAuthProvider:
type OAuthProvider struct {
    // ...
    NoBrowser bool
}
  1. gate the open (~line 396):
if !p.NoBrowser {
    if err := openBrowser(authURL); err != nil && p.logger != nil {
        p.logger.Warn(i18n.T("无法自动打开浏览器"), "error", err)
    }
}
  1. internal/app/auth_command.go — wire it into the default: (OAuth) branch, same as the device branch:
provider := authpkg.NewOAuthProvider(configDir, nil)
provider.Output = cmd.ErrOrStderr()
provider.NoBrowser, _ = cmd.Flags().GetBool("no-browser")
configureOAuthProviderCompatibility(provider, configDir)

With that, --no-browser works for both --device and the default login. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants