Cleanup code slightly

Author: Askir

internal/tiger/common/password_storage.go

@@ -2,6 +2,7 @@ package common
 
 import (
 	"bufio"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -339,26 +340,44 @@ type AutoFallbackStorage struct {
 
 func (a *AutoFallbackStorage) Save(service api.Service, password string, role string) error {
 	// Try keyring first
-	if err := a.keyring.Save(service, password, role); err == nil {
-		a.lastMethod = "keyring"
+	a.lastMethod = "keyring"
+	keyringErr := a.keyring.Save(service, password, role)
+	if keyringErr == nil {
 		return nil
 	}
+
 	// Any keyring error -> fall back to pgpass
-	if err := a.pgpass.Save(service, password, role); err != nil {
-		a.lastMethod = "auto"
-		return err
-	}
 	a.lastMethod = "pgpass"
-	return nil
+	pgpassErr := a.pgpass.Save(service, password, role)
+	if pgpassErr == nil {
+		return nil
+	}
+
+	// Both failed - return combined error
+	return errors.Join(
+		fmt.Errorf("keyring: %w", keyringErr),
+		fmt.Errorf("pgpass: %w", pgpassErr),
+	)
 }
 
 func (a *AutoFallbackStorage) Get(service api.Service, role string) (string, error) {
 	// Try keyring first
-	if password, err := a.keyring.Get(service, role); err == nil {
+	password, keyringErr := a.keyring.Get(service, role)
+	if keyringErr == nil {
 		return password, nil
 	}
+
 	// Any keyring error -> try pgpass
-	return a.pgpass.Get(service, role)
+	password, pgpassErr := a.pgpass.Get(service, role)
+	if pgpassErr == nil {
+		return password, nil
+	}
+
+	// Both failed
+	return "", errors.Join(
+		fmt.Errorf("keyring: %w", keyringErr),
+		fmt.Errorf("pgpass: %w", pgpassErr),
+	)
 }
 
 func (a *AutoFallbackStorage) Remove(service api.Service, role string) error {
@@ -370,8 +389,11 @@ func (a *AutoFallbackStorage) Remove(service api.Service, role string) error {
 	if keyringErr == nil || pgpassErr == nil {
 		return nil
 	}
-	// If both failed, return a combined error
-	return fmt.Errorf("keyring: %v, pgpass: %v", keyringErr, pgpassErr)
+	// Both failed
+	return errors.Join(
+		fmt.Errorf("keyring: %w", keyringErr),
+		fmt.Errorf("pgpass: %w", pgpassErr),
+	)
 }
 
 func (a *AutoFallbackStorage) GetStorageResult(err error, password string) PasswordStorageResult {
@@ -391,18 +413,12 @@ func (a *AutoFallbackStorage) GetStorageResult(err error, password string) Passw
 			Method:  "keyring",
 			Message: "Password saved to system keyring for automatic authentication",
 		}
-	case "pgpass":
+	default:
 		return PasswordStorageResult{
 			Success: true,
 			Method:  "pgpass",
 			Message: "Password saved to ~/.pgpass for automatic authentication",
 		}
-	default:
-		return PasswordStorageResult{
-			Success: true,
-			Method:  "auto",
-			Message: "Password saved for automatic authentication",
-		}
 	}
 }
 
@@ -416,13 +432,7 @@ func GetPasswordStorage() PasswordStorage {
 		return &PgpassStorage{}
 	case "none":
 		return &NoStorage{}
-	case "auto":
-		return &AutoFallbackStorage{
-			keyring: &KeyringStorage{},
-			pgpass:  &PgpassStorage{},
-		}
 	default:
-		// Default to auto for best compatibility across environments
 		return &AutoFallbackStorage{
 			keyring: &KeyringStorage{},
 			pgpass:  &PgpassStorage{},

internal/tiger/common/password_storage_test.go

@@ -296,7 +296,7 @@ func TestGetPasswordStorage(t *testing.T) {
 		{"pgpass", "pgpass", "*common.PgpassStorage"},
 		{"none", "none", "*common.NoStorage"},
 		{"auto", "auto", "*common.AutoFallbackStorage"},
-		{"default", "", "*common.AutoFallbackStorage"},        // Default case now uses auto
+		{"default", "", "*common.AutoFallbackStorage"},
 		{"invalid", "invalid", "*common.AutoFallbackStorage"}, // Falls back to auto
 	}
 
@@ -990,134 +990,3 @@ func TestAutoFallbackStorage_Integration(t *testing.T) {
 		t.Error("AutoFallbackStorage.Get() should fail after Remove()")
 	}
 }
-
-// Test AutoFallbackStorage GetStorageResult with keyring success
-func TestAutoFallbackStorage_GetStorageResult_KeyringSuccess(t *testing.T) {
-	storage := &AutoFallbackStorage{
-		keyring:    &KeyringStorage{},
-		pgpass:     &PgpassStorage{},
-		lastMethod: "keyring",
-	}
-	testPassword := "test-password"
-
-	result := storage.GetStorageResult(nil, testPassword)
-	if !result.Success {
-		t.Errorf("GetStorageResult() success should be true, got: %t", result.Success)
-	}
-	if result.Method != "keyring" {
-		t.Errorf("GetStorageResult() method should be 'keyring', got: %s", result.Method)
-	}
-	if !strings.Contains(result.Message, "Password saved to system keyring") {
-		t.Errorf("GetStorageResult() should mention keyring, got: %s", result.Message)
-	}
-	if strings.Contains(result.Message, testPassword) {
-		t.Error("GetStorageResult() should never include actual passwords")
-	}
-}
-
-// Test AutoFallbackStorage GetStorageResult with pgpass fallback success
-func TestAutoFallbackStorage_GetStorageResult_PgpassSuccess(t *testing.T) {
-	storage := &AutoFallbackStorage{
-		keyring:    &KeyringStorage{},
-		pgpass:     &PgpassStorage{},
-		lastMethod: "pgpass",
-	}
-	testPassword := "test-password"
-
-	result := storage.GetStorageResult(nil, testPassword)
-	if !result.Success {
-		t.Errorf("GetStorageResult() success should be true, got: %t", result.Success)
-	}
-	if result.Method != "pgpass" {
-		t.Errorf("GetStorageResult() method should be 'pgpass', got: %s", result.Method)
-	}
-	if !strings.Contains(result.Message, "Password saved to ~/.pgpass") {
-		t.Errorf("GetStorageResult() should mention pgpass, got: %s", result.Message)
-	}
-	if strings.Contains(result.Message, testPassword) {
-		t.Error("GetStorageResult() should never include actual passwords")
-	}
-}
-
-// Test AutoFallbackStorage GetStorageResult with failure
-func TestAutoFallbackStorage_GetStorageResult_Failure(t *testing.T) {
-	storage := &AutoFallbackStorage{
-		keyring:    &KeyringStorage{},
-		pgpass:     &PgpassStorage{},
-		lastMethod: "auto",
-	}
-	testPassword := "test-password"
-	testError := fmt.Errorf("both storage methods failed")
-
-	result := storage.GetStorageResult(testError, testPassword)
-	if result.Success {
-		t.Errorf("GetStorageResult() success should be false, got: %t", result.Success)
-	}
-	if result.Method != "auto" {
-		t.Errorf("GetStorageResult() method should be 'auto', got: %s", result.Method)
-	}
-	if !strings.Contains(result.Message, "Failed to save password") {
-		t.Errorf("GetStorageResult() should mention failure, got: %s", result.Message)
-	}
-	if strings.Contains(result.Message, testPassword) {
-		t.Error("GetStorageResult() should never include actual passwords")
-	}
-}
-
-// Test AutoFallbackStorage Remove succeeds if at least one location succeeds
-func TestAutoFallbackStorage_Remove_PartialSuccess(t *testing.T) {
-	// Set up test service name for keyring
-	config.SetTestServiceName(t)
-
-	// Create a temporary directory for pgpass
-	tempDir := t.TempDir()
-	originalHome := os.Getenv("HOME")
-	os.Setenv("HOME", tempDir)
-	defer os.Setenv("HOME", originalHome)
-
-	// Save password only to pgpass (not keyring)
-	pgpassStorage := &PgpassStorage{}
-	service := createTestService("partial-remove-test")
-	password := "test-password"
-	role := "tsdbadmin"
-
-	err := pgpassStorage.Save(service, password, role)
-	if err != nil {
-		t.Fatalf("PgpassStorage.Save() failed: %v", err)
-	}
-
-	// Now remove using AutoFallbackStorage - should succeed even if keyring fails
-	autoStorage := &AutoFallbackStorage{
-		keyring: &KeyringStorage{},
-		pgpass:  &PgpassStorage{},
-	}
-
-	err = autoStorage.Remove(service, role)
-	if err != nil {
-		t.Errorf("AutoFallbackStorage.Remove() should succeed when at least one location succeeds, got: %v", err)
-	}
-}
-
-// Security test: Ensure AutoFallbackStorage never includes passwords in messages
-func TestAutoFallbackStorage_SecurityTest_NoPasswordInResults(t *testing.T) {
-	testPassword := "super-secret-password-123"
-	testError := fmt.Errorf("failed to save %s", testPassword)
-
-	storage := &AutoFallbackStorage{
-		keyring:    &KeyringStorage{},
-		pgpass:     &PgpassStorage{},
-		lastMethod: "auto",
-	}
-
-	// Test success case
-	successResult := storage.GetStorageResult(nil, testPassword)
-	if strings.Contains(successResult.Message, testPassword) {
-		t.Errorf("GetStorageResult() success should never include password, got: %s", successResult.Message)
-	}
-
-	// Test error case
-	errorResult := storage.GetStorageResult(testError, testPassword)
-	if strings.Contains(errorResult.Message, testPassword) {
-		t.Errorf("GetStorageResult() error should never include password, got: %s", errorResult.Message)
-	}
-}