chore: tests cleanup

Author: denis256

internal/gcphelper/config.go

@@ -22,6 +22,9 @@ import (
 )
 
 const (
+	// envNameGoogleApplicationCredentials names the file holding Application Default Credentials.
+	envNameGoogleApplicationCredentials = "GOOGLE_APPLICATION_CREDENTIALS"
+
 	tokenURL = "https://oauth2.googleapis.com/token"
 
 	cloudPlatformScope = "https://www.googleapis.com/auth/cloud-platform"
@@ -134,9 +137,14 @@ func (b *GCPConfigBuilder) Build(
 		return nil, err
 	}
 
-	if envCreds != nil {
+	switch {
+	case envCreds != nil:
 		clientOpts = append(clientOpts, envCreds)
-	} else if gcpCfg != nil && gcpCfg.Credentials != "" {
+	// GOOGLE_APPLICATION_CREDENTIALS named a file that read as empty. Fall through to the
+	// ADC chain rather than to the remaining sources, which would authenticate as a
+	// different identity than the one the user pointed at.
+	case env[envNameGoogleApplicationCredentials] != "":
+	case gcpCfg != nil && gcpCfg.Credentials != "":
 		// Use credentials file from config
 		credOpt, err := credentialsFileOption(v, gcpCfg.Credentials)
 		if err != nil {
@@ -146,19 +154,19 @@ func (b *GCPConfigBuilder) Build(
 		if credOpt != nil {
 			clientOpts = append(clientOpts, credOpt)
 		}
-	} else if gcpCfg != nil && gcpCfg.AccessToken != "" {
+	case gcpCfg != nil && gcpCfg.AccessToken != "":
 		// Use access token from config
 		tokenSource := oauth2.StaticTokenSource(&oauth2.Token{
 			AccessToken: gcpCfg.AccessToken,
 		})
 		clientOpts = append(clientOpts, option.WithTokenSource(tokenSource))
-	} else if oauthAccessToken := env["GOOGLE_OAUTH_ACCESS_TOKEN"]; oauthAccessToken != "" {
+	case env["GOOGLE_OAUTH_ACCESS_TOKEN"] != "":
 		// Use OAuth access token from environment
 		tokenSource := oauth2.StaticTokenSource(&oauth2.Token{
-			AccessToken: oauthAccessToken,
+			AccessToken: env["GOOGLE_OAUTH_ACCESS_TOKEN"],
 		})
 		clientOpts = append(clientOpts, option.WithTokenSource(tokenSource))
-	} else if env["GOOGLE_CREDENTIALS"] != "" {
+	case env["GOOGLE_CREDENTIALS"] != "":
 		// Use GOOGLE_CREDENTIALS from environment (can be file path or JSON content)
 		clientOpt, err := createGCPCredentialsFromGoogleCredentialsEnv(ctx, v)
 		if err != nil {
@@ -194,7 +202,7 @@ func (b *GCPConfigBuilder) Build(
 // GOOGLE_APPLICATION_CREDENTIALS variable in v's environment. Returns nil when
 // the variable is not set.
 func createGCPCredentialsFromEnv(v *venv.Venv) (option.ClientOption, error) {
-	credentialsFile := v.Env["GOOGLE_APPLICATION_CREDENTIALS"]
+	credentialsFile := v.Env[envNameGoogleApplicationCredentials]
 	if credentialsFile == "" {
 		return nil, nil
 	}

internal/gcphelper/config_test.go

@@ -292,3 +292,23 @@ func TestGcpConfigEmptyCredentialsFileFallsBackToADC(t *testing.T) {
 	require.NoError(t, err)
 	assert.Empty(t, clientOpts)
 }
+
+// TestGcpConfigEmptyGACDoesNotFallBackToGoogleCredentials pins that an unpopulated
+// GOOGLE_APPLICATION_CREDENTIALS file falls through to ADC, not to a leftover
+// GOOGLE_CREDENTIALS naming a different service account.
+func TestGcpConfigEmptyGACDoesNotFallBackToGoogleCredentials(t *testing.T) {
+	t.Parallel()
+
+	gacFile := filepath.Join(t.TempDir(), "gac.json")
+	require.NoError(t, os.WriteFile(gacFile, nil, 0o600))
+
+	env := map[string]string{
+		"GOOGLE_APPLICATION_CREDENTIALS": gacFile,
+		"GOOGLE_CREDENTIALS":             string(serviceAccountJSON(t)),
+	}
+
+	clientOpts, err := gcphelper.NewGCPConfigBuilder().
+		Build(context.Background(), venvtest.NewWithOSFS().WithEnv(env))
+	require.NoError(t, err)
+	assert.Empty(t, clientOpts, "leftover GOOGLE_CREDENTIALS must not win over an empty GAC file")
+}

internal/tf/cliconfig/user_config.go

@@ -68,12 +68,18 @@ func LoadUserConfig(v *venv.Venv, opts ...ConfigOption) (*Config, error) {
 
 	config := NewConfig(v.FS).WithProviderInstallation(&ProviderInstallation{})
 
+	var helperSources []string
+
 	for _, path := range paths {
 		fileConfig, err := loadUserConfigFile(v, path)
 		if err != nil {
 			return nil, err
 		}
 
+		if fileConfig.CredentialsHelpers != nil {
+			helperSources = append(helperSources, path)
+		}
+
 		mergeUserConfig(config, fileConfig)
 	}
 
@@ -82,7 +88,7 @@ func LoadUserConfig(v *venv.Venv, opts ...ConfigOption) (*Config, error) {
 		config.PluginCacheDir = pluginCacheDir
 	}
 
-	if err := validateUserConfig(config); err != nil {
+	if err := validateUserConfig(config, helperSources); err != nil {
 		return nil, err
 	}
 
@@ -195,6 +201,15 @@ func loadUserConfigFile(v *venv.Venv, path string) (*Config, error) {
 		return nil, fmt.Errorf("%w: decoding %s: %w", ErrUserConfig, path, err)
 	}
 
+	// Ranging a map to pick "the" helper would decide by iteration order, so reject the
+	// ambiguity the way OpenTofu's Config.Validate does.
+	if len(file.CredentialsHelpers) > 1 {
+		return nil, fmt.Errorf(
+			"%w: no more than one credentials_helper block may be specified, %s declares %d",
+			ErrInvalidUserConfig, path, len(file.CredentialsHelpers),
+		)
+	}
+
 	methods, err := decodeProviderInstallation(path, node)
 	if err != nil {
 		return nil, err
@@ -307,8 +322,16 @@ func expandUserConfigEnv(value string, env map[string]string) string {
 }
 
 // validateUserConfig rejects the malformed blocks OpenTofu rejects, so a bad hostname surfaces
-// here rather than as an unauthenticated registry request later.
-func validateUserConfig(config *Config) error {
+// here rather than as an unauthenticated registry request later. helperSources names every
+// file that declared a credentials_helper, which upstream allows only once across them all.
+func validateUserConfig(config *Config, helperSources []string) error {
+	if len(helperSources) > 1 {
+		return fmt.Errorf(
+			"%w: no more than one credentials_helper block may be specified, found one in each of %s",
+			ErrInvalidUserConfig, strings.Join(helperSources, ", "),
+		)
+	}
+
 	for _, creds := range config.Credentials {
 		if _, err := svchost.ForComparison(creds.Name); err != nil {
 			return fmt.Errorf(
@@ -373,7 +396,8 @@ func userCredentials(file userConfigFile) []ConfigCredentials {
 	return credentials
 }
 
-// userCredentialsHelper returns the single credentials_helper block, or nil when the file declares none.
+// userCredentialsHelper returns the single credentials_helper block, or nil when the file
+// declares none. More than one is rejected by the caller, so the map holds at most one entry.
 func userCredentialsHelper(file userConfigFile) *ConfigCredentialsHelper {
 	for name, helper := range file.CredentialsHelpers {
 		var args []string

internal/tf/cliconfig/user_config_test.go

@@ -297,6 +297,11 @@ func TestLoadUserConfig_Errors(t *testing.T) {
 			source:   "provider_installation {\n  bogus_mirror {}\n}\n",
 			expected: cliconfig.ErrInvalidUserConfig,
 		},
+		{
+			name:     "two credentials helpers in one file",
+			source:   "credentials_helper \"vault\" {\n  args = [\"token\"]\n}\n\ncredentials_helper \"oskeychain\" {\n}\n",
+			expected: cliconfig.ErrInvalidUserConfig,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -349,3 +354,28 @@ func userConfigVenv(home string, env map[string]string) *venv.Venv {
 		WithUserHomeDir(func() (string, error) { return home, nil }).
 		WithEnv(env)
 }
+
+// TestLoadUserConfig_CredentialsHelperAcrossFiles pins that a second helper in a fragment is
+// rejected too, rather than silently overriding the one in the main file.
+func TestLoadUserConfig_CredentialsHelperAcrossFiles(t *testing.T) {
+	t.Parallel()
+
+	const home = "/virtual/home"
+
+	configDir := filepath.Join(home, ".terraform.d")
+	v := userConfigVenv(home, nil)
+
+	require.NoError(t, v.FS.MkdirAll(configDir, 0o755))
+	require.NoError(t, vfs.WriteFile(v.FS, filepath.Join(home, ".tofurc"), []byte(`
+credentials_helper "vault" {
+  args = ["token"]
+}
+`), 0o600))
+	require.NoError(t, vfs.WriteFile(v.FS, filepath.Join(configDir, "10-extra.tfrc"), []byte(`
+credentials_helper "oskeychain" {
+}
+`), 0o600))
+
+	_, err := cliconfig.LoadUserConfig(v)
+	require.ErrorIs(t, err, cliconfig.ErrInvalidUserConfig)
+}