Skip to content

Commit 8430dd6

Browse files
zedkippzedkipp
authored
fix(cli): remove defaulting to keyring when --global-config set (cherry-pick) (#20953)
(cherry picked from commit bbf7b13)
1 parent 0bd0990 commit 8430dd6

File tree

6 files changed

+38
-20
lines changed

6 files changed

+38
-20
lines changed

‎cli/clitest/clitest.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,10 @@ func NewWithCommand(
6161
t testing.TB, cmd *serpent.Command, args ...string,
6262
) (*serpent.Invocation, config.Root) {
6363
configDir := config.Root(t.TempDir())
64-
// Keyring usage is disabled here because many existing tests expect the session token
65-
// to be stored on disk and is not properly instrumented for parallel testing against
66-
// the actual operating system keyring.
67-
invArgs := append([]string{"--global-config", string(configDir), "--use-keyring=false"}, args...)
64+
// Keyring usage is disabled here when --global-config is set because many existing
65+
// tests expect the session token to be stored on disk and is not properly instrumented
66+
// for parallel testing against the actual operating system keyring.
67+
invArgs := append([]string{"--global-config", string(configDir)}, args...)
6868
return setupInvocation(t, cmd, invArgs...), configDir
6969
}
7070

‎cli/keyring_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ func setupKeyringTestEnv(t *testing.T, clientURL string, args ...string) keyring
5454

5555
serviceName := keyringTestServiceName(t)
5656
root.WithKeyringServiceName(serviceName)
57+
root.UseKeyringWithGlobalConfig()
5758

5859
inv, cfg := clitest.NewWithDefaultKeyringCommand(t, cmd, args...)
5960

@@ -169,6 +170,7 @@ func TestUseKeyring(t *testing.T) {
169170
logoutCmd, err := logoutRoot.Command(logoutRoot.AGPL())
170171
require.NoError(t, err)
171172
logoutRoot.WithKeyringServiceName(env.serviceName)
173+
logoutRoot.UseKeyringWithGlobalConfig()
172174

173175
logoutInv, _ := clitest.NewWithDefaultKeyringCommand(t, logoutCmd,
174176
"logout",

‎cli/root.go‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -484,9 +484,9 @@ func (r *RootCmd) Command(subcommands []*serpent.Command) (*serpent.Command, err
484484
Flag: varUseKeyring,
485485
Env: envUseKeyring,
486486
Description: "Store and retrieve session tokens using the operating system " +
487-
"keyring. Enabled by default. If the keyring is not supported on the " +
488-
"current platform, file-based storage is used automatically. Set to " +
489-
"false to force file-based storage.",
487+
"keyring. This flag is ignored and file-based storage is used when " +
488+
"--global-config is set or keyring usage is not supported on the current " +
489+
"platform. Set to false to force file-based storage on supported platforms.",
490490
Default: "true",
491491
Value: serpent.BoolOf(&r.useKeyring),
492492
Group: globalGroup,
@@ -537,11 +537,12 @@ type RootCmd struct {
537537
disableDirect bool
538538
debugHTTP bool
539539

540-
disableNetworkTelemetry bool
541-
noVersionCheck bool
542-
noFeatureWarning bool
543-
useKeyring bool
544-
keyringServiceName string
540+
disableNetworkTelemetry bool
541+
noVersionCheck bool
542+
noFeatureWarning bool
543+
useKeyring bool
544+
keyringServiceName string
545+
useKeyringWithGlobalConfig bool
545546
}
546547

547548
// InitClient creates and configures a new client with authentication, telemetry,
@@ -722,8 +723,14 @@ func (r *RootCmd) createUnauthenticatedClient(ctx context.Context, serverURL *ur
722723
// flag.
723724
func (r *RootCmd) ensureTokenBackend() sessionstore.Backend {
724725
if r.tokenBackend == nil {
726+
// Checking for the --global-config directory being set is a bit wonky but necessary
727+
// to allow extensions that invoke the CLI with this flag (e.g. VS code) to continue
728+
// working without modification. In the future we should modify these extensions to
729+
// either access the credential in the keyring (like Coder Desktop) or some other
730+
// approach that doesn't rely on the session token being stored on disk.
731+
assumeExtensionInUse := r.globalConfig != config.DefaultDir() && !r.useKeyringWithGlobalConfig
725732
keyringSupported := runtime.GOOS == "windows" || runtime.GOOS == "darwin"
726-
if r.useKeyring && keyringSupported {
733+
if r.useKeyring && !assumeExtensionInUse && keyringSupported {
727734
serviceName := sessionstore.DefaultServiceName
728735
if r.keyringServiceName != "" {
729736
serviceName = r.keyringServiceName
@@ -743,6 +750,13 @@ func (r *RootCmd) WithKeyringServiceName(serviceName string) {
743750
r.keyringServiceName = serviceName
744751
}
745752

753+
// UseKeyringWithGlobalConfig enables the use of the keyring storage backend
754+
// when the --global-config directory is set. This is only intended as an override
755+
// for tests, which require specifying the global config directory for test isolation.
756+
func (r *RootCmd) UseKeyringWithGlobalConfig() {
757+
r.useKeyringWithGlobalConfig = true
758+
}
759+
746760
type AgentAuth struct {
747761
// Agent Client config
748762
agentToken string

‎cli/testdata/coder_--help.golden‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,10 @@ variables or flags.
111111

112112
--use-keyring bool, $CODER_USE_KEYRING (default: true)
113113
Store and retrieve session tokens using the operating system keyring.
114-
Enabled by default. If the keyring is not supported on the current
115-
platform, file-based storage is used automatically. Set to false to
116-
force file-based storage.
114+
This flag is ignored and file-based storage is used when
115+
--global-config is set or keyring usage is not supported on the
116+
current platform. Set to false to force file-based storage on
117+
supported platforms.
117118

118119
-v, --verbose bool, $CODER_VERBOSE
119120
Enable verbose output.

‎docs/reference/cli/index.md‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎enterprise/cli/testdata/coder_--help.golden‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,10 @@ variables or flags.
7070

7171
--use-keyring bool, $CODER_USE_KEYRING (default: true)
7272
Store and retrieve session tokens using the operating system keyring.
73-
Enabled by default. If the keyring is not supported on the current
74-
platform, file-based storage is used automatically. Set to false to
75-
force file-based storage.
73+
This flag is ignored and file-based storage is used when
74+
--global-config is set or keyring usage is not supported on the
75+
current platform. Set to false to force file-based storage on
76+
supported platforms.
7677

7778
-v, --verbose bool, $CODER_VERBOSE
7879
Enable verbose output.

0 commit comments

Comments
 (0)