fix(cli): remove defaulting to keyring when --global-config set (#20943)

This fixes a regression that caused the VS code extension to be unable
to authenticate after making keyring usage on by default. This is
because the VS code extension assumes the CLI will always use the
session token stored on disk, specifically in the directory specified by
--global-config.

This fix makes keyring usage enabled when the --global-config directory
is not set. This is a bit wonky but necessary to allow the extension to
continue working without modification and without backwards compat
concerns. In the future we should modify these extensions to either
access the credential in the keyring (like Coder Desktop) or some other
approach that doesn't rely on the session token being stored on disk.

Tests:
`coder login dev.coder.com` -> token stored in keyring
`coder login --global-config=/tmp/ dev.coder.com` -> token stored in
`/tmp/session`

Author: zedkipp

cli/clitest/clitest.go

@@ -61,10 +61,10 @@ func NewWithCommand(
 	t testing.TB, cmd *serpent.Command, args ...string,
 ) (*serpent.Invocation, config.Root) {
 	configDir := config.Root(t.TempDir())
-	// Keyring usage is disabled here because many existing tests expect the session token
-	// to be stored on disk and is not properly instrumented for parallel testing against
-	// the actual operating system keyring.
-	invArgs := append([]string{"--global-config", string(configDir), "--use-keyring=false"}, args...)
+	// Keyring usage is disabled here when --global-config is set because many existing
+	// tests expect the session token to be stored on disk and is not properly instrumented
+	// for parallel testing against the actual operating system keyring.
+	invArgs := append([]string{"--global-config", string(configDir)}, args...)
 	return setupInvocation(t, cmd, invArgs...), configDir
 }
 

cli/keyring_test.go

@@ -54,6 +54,7 @@ func setupKeyringTestEnv(t *testing.T, clientURL string, args ...string) keyring
 
 	serviceName := keyringTestServiceName(t)
 	root.WithKeyringServiceName(serviceName)
+	root.UseKeyringWithGlobalConfig()
 
 	inv, cfg := clitest.NewWithDefaultKeyringCommand(t, cmd, args...)
 
@@ -169,6 +170,7 @@ func TestUseKeyring(t *testing.T) {
 		logoutCmd, err := logoutRoot.Command(logoutRoot.AGPL())
 		require.NoError(t, err)
 		logoutRoot.WithKeyringServiceName(env.serviceName)
+		logoutRoot.UseKeyringWithGlobalConfig()
 
 		logoutInv, _ := clitest.NewWithDefaultKeyringCommand(t, logoutCmd,
 			"logout",

cli/root.go

@@ -483,9 +483,9 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
 			Flag: varUseKeyring,
 			Env:  envUseKeyring,
 			Description: "Store and retrieve session tokens using the operating system " +
-				"keyring. Enabled by default. If the keyring is not supported on the " +
-				"current platform, file-based storage is used automatically. Set to " +
-				"false to force file-based storage.",
+				"keyring. This flag is ignored and file-based storage is used when " +
+				"--global-config is set or keyring usage is not supported on the current " +
+				"platform. Set to false to force file-based storage on supported platforms.",
 			Default: "true",
 			Value:   serpent.BoolOf(&r.useKeyring),
 			Group:   globalGroup,
@@ -536,11 +536,12 @@ type RootCmd struct {
 	disableDirect bool
 	debugHTTP     bool
 
-	disableNetworkTelemetry bool
-	noVersionCheck          bool
-	noFeatureWarning        bool
-	useKeyring              bool
-	keyringServiceName      string
+	disableNetworkTelemetry    bool
+	noVersionCheck             bool
+	noFeatureWarning           bool
+	useKeyring                 bool
+	keyringServiceName         string
+	useKeyringWithGlobalConfig bool
 }
 
 // InitClient creates and configures a new client with authentication, telemetry,
@@ -721,8 +722,14 @@ func (r *RootCmd) createUnauthenticatedClient(ctx context.Context, serverURL *ur
 // flag.
 func (r *RootCmd) ensureTokenBackend() sessionstore.Backend {
 	if r.tokenBackend == nil {
+		// Checking for the --global-config directory being set is a bit wonky but necessary
+		// to allow extensions that invoke the CLI with this flag (e.g. VS code) to continue
+		// working without modification. In the future we should modify these extensions to
+		// either access the credential in the keyring (like Coder Desktop) or some other
+		// approach that doesn't rely on the session token being stored on disk.
+		assumeExtensionInUse := r.globalConfig != config.DefaultDir() && !r.useKeyringWithGlobalConfig
 		keyringSupported := runtime.GOOS == "windows" || runtime.GOOS == "darwin"
-		if r.useKeyring && keyringSupported {
+		if r.useKeyring && !assumeExtensionInUse && keyringSupported {
 			serviceName := sessionstore.DefaultServiceName
 			if r.keyringServiceName != "" {
 				serviceName = r.keyringServiceName
@@ -742,6 +749,13 @@ func (r *RootCmd) WithKeyringServiceName(serviceName string) {
 	r.keyringServiceName = serviceName
 }
 
+// UseKeyringWithGlobalConfig enables the use of the keyring storage backend
+// when the --global-config directory is set. This is only intended as an override
+// for tests, which require specifying the global config directory for test isolation.
+func (r *RootCmd) UseKeyringWithGlobalConfig() {
+	r.useKeyringWithGlobalConfig = true
+}
+
 type AgentAuth struct {
 	// Agent Client config
 	agentToken     string

cli/testdata/coder_--help.golden

@@ -111,9 +111,10 @@ variables or flags.
 
       --use-keyring bool, $CODER_USE_KEYRING (default: true)
           Store and retrieve session tokens using the operating system keyring.
-          Enabled by default. If the keyring is not supported on the current
-          platform, file-based storage is used automatically. Set to false to
-          force file-based storage.
+          This flag is ignored and file-based storage is used when
+          --global-config is set or keyring usage is not supported on the
+          current platform. Set to false to force file-based storage on
+          supported platforms.
 
   -v, --verbose bool, $CODER_VERBOSE
           Enable verbose output.

docs/reference/cli/index.md

@@ -70,9 +70,10 @@ variables or flags.
 
       --use-keyring bool, $CODER_USE_KEYRING (default: true)
           Store and retrieve session tokens using the operating system keyring.
-          Enabled by default. If the keyring is not supported on the current
-          platform, file-based storage is used automatically. Set to false to
-          force file-based storage.
+          This flag is ignored and file-based storage is used when
+          --global-config is set or keyring usage is not supported on the
+          current platform. Set to false to force file-based storage on
+          supported platforms.
 
   -v, --verbose bool, $CODER_VERBOSE
           Enable verbose output.