From 1cec38fb4df2b40d8ec4cbfb733a85c3fdafff11 Mon Sep 17 00:00:00 2001 From: Matvey Arye Date: Wed, 15 Oct 2025 11:07:28 +0200 Subject: [PATCH 1/4] Add TIGER_NEW_PASSWORD env var and interactive --password flag to save-password spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update the save-password command specification to support: - TIGER_NEW_PASSWORD environment variable (used when --password flag not provided) - Interactive password prompt when --password flag provided with no value - Clear precedence: explicit flag value → interactive prompt → env var 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- specs/spec.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/specs/spec.md b/specs/spec.md index 2c099fa6..405543e6 100644 --- a/specs/spec.md +++ b/specs/spec.md @@ -31,6 +31,7 @@ mv tiger /usr/local/bin/ - `TIGER_CONFIG_DIR`: Configuration directory (default: ~/.config/tiger) - `TIGER_OUTPUT`: Default output format (json, yaml, table) - `TIGER_ANALYTICS`: Enable/disable usage analytics (true/false) +- `TIGER_NEW_PASSWORD`: Password to use for save-password command (overridden by --password flag) ### Configuration File @@ -396,10 +397,17 @@ tiger db test-connection svc-12345 # Test with custom timeout tiger db test-connection svc-12345 --timeout 10s -# Save password (storage location depends on --password-storage flag) +# Save password with explicit value (highest precedence) tiger db save-password svc-12345 --password your-password tiger db save-password svc-12345 --password your-password --role readonly +# Interactive password prompt (when --password flag provided with no value) +tiger db save-password svc-12345 --password + +# Using environment variable (only when --password flag not provided) +export TIGER_NEW_PASSWORD=your-password +tiger db save-password svc-12345 + # Save to specific storage location tiger db save-password svc-12345 --password your-password --password-storage pgpass tiger db save-password svc-12345 --password your-password --password-storage keyring @@ -428,7 +436,7 @@ The `connect` and `psql` commands support passing additional flags directly to t **Options:** - `--pooled`: Use connection pooling (for connection-string command) - `--role`: Database role to use (default: tsdbadmin) -- `--password`: Password to save (for save-password command) +- `--password`: Password to save (for save-password command). When flag is provided with no value, prompts interactively. When flag is not provided at all, uses TIGER_NEW_PASSWORD environment variable if set. - `-t, --timeout`: Timeout for test-connection (accepts any duration format from Go's time.ParseDuration: "3s", "30s", "1m", etc.) (default: 3s, set to 0 to disable) ### High-Availability Management From 6cabab5e5ca05f96d7bf1034b218f374aeeaac8f Mon Sep 17 00:00:00 2001 From: Matvey Arye Date: Wed, 15 Oct 2025 12:18:22 +0200 Subject: [PATCH 2/4] Add role parameter to password storage system MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement role-based password storage to support storing passwords for different database roles (tsdbadmin, readonly, custom roles, etc.) independently. This enables users to manage multiple credentials per service for different database users. Key changes: - Add role parameter to PasswordStorage interface (Save, Get, Remove) - Update KeyringStorage to use role in keyring keys - Update PgpassStorage to store role in pgpass entries - Update all callers in cmd, mcp, and password packages - Add comprehensive tests including mixed-case role names - Verify role isolation (different roles store passwords independently) All tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/tiger/cmd/db.go | 10 +- internal/tiger/cmd/db_test.go | 16 +- internal/tiger/cmd/service.go | 2 +- internal/tiger/mcp/service_tools.go | 4 +- internal/tiger/password/connection.go | 6 +- internal/tiger/password/connection_test.go | 10 +- internal/tiger/password/password_storage.go | 65 +++-- .../tiger/password/password_storage_test.go | 275 ++++++++++++++++-- 8 files changed, 317 insertions(+), 71 deletions(-) diff --git a/internal/tiger/cmd/db.go b/internal/tiger/cmd/db.go index 4e60231e..8527de81 100644 --- a/internal/tiger/cmd/db.go +++ b/internal/tiger/cmd/db.go @@ -157,7 +157,7 @@ Examples: } // Launch psql with additional flags - return launchPsqlWithConnectionString(details.String(), psqlPath, psqlFlags, service, cmd) + return launchPsqlWithConnectionString(details.String(), psqlPath, psqlFlags, service, dbConnectRole, cmd) }, } @@ -339,13 +339,13 @@ func separateServiceAndPsqlArgs(cmd ArgsLenAtDashProvider, args []string) ([]str } // launchPsqlWithConnectionString launches psql using the connection string and additional flags -func launchPsqlWithConnectionString(connectionString, psqlPath string, additionalFlags []string, service api.Service, cmd *cobra.Command) error { - psqlCmd := buildPsqlCommand(connectionString, psqlPath, additionalFlags, service, cmd) +func launchPsqlWithConnectionString(connectionString, psqlPath string, additionalFlags []string, service api.Service, role string, cmd *cobra.Command) error { + psqlCmd := buildPsqlCommand(connectionString, psqlPath, additionalFlags, service, role, cmd) return psqlCmd.Run() } // buildPsqlCommand creates the psql command with proper environment setup -func buildPsqlCommand(connectionString, psqlPath string, additionalFlags []string, service api.Service, cmd *cobra.Command) *exec.Cmd { +func buildPsqlCommand(connectionString, psqlPath string, additionalFlags []string, service api.Service, role string, cmd *cobra.Command) *exec.Cmd { // Build command arguments: connection string first, then additional flags // Note: connectionString contains only "postgresql://user@host:port/db" - no password // Passwords are passed via PGPASSWORD environment variable (see below) @@ -363,7 +363,7 @@ func buildPsqlCommand(connectionString, psqlPath string, additionalFlags []strin // pgpass storage relies on psql automatically reading ~/.pgpass file storage := password.GetPasswordStorage() if _, isKeyring := storage.(*password.KeyringStorage); isKeyring { - if password, err := storage.Get(service); err == nil && password != "" { + if password, err := storage.Get(service, role); err == nil && password != "" { // Set PGPASSWORD environment variable for psql when using keyring psqlCmd.Env = append(os.Environ(), "PGPASSWORD="+password) } diff --git a/internal/tiger/cmd/db_test.go b/internal/tiger/cmd/db_test.go index 98937f2f..5d5bd0a4 100644 --- a/internal/tiger/cmd/db_test.go +++ b/internal/tiger/cmd/db_test.go @@ -271,7 +271,7 @@ func TestLaunchPsqlWithConnectionString(t *testing.T) { service := api.Service{} // This will fail because psql path doesn't exist, but we can verify the error - err := launchPsqlWithConnectionString(connectionString, psqlPath, []string{}, service, cmd) + err := launchPsqlWithConnectionString(connectionString, psqlPath, []string{}, service, "tsdbadmin", cmd) // Should fail with exec error since fake psql path doesn't exist if err == nil { @@ -301,7 +301,7 @@ func TestLaunchPsqlWithAdditionalFlags(t *testing.T) { service := api.Service{} // This will fail because psql path doesn't exist, but we can verify the error - err := launchPsqlWithConnectionString(connectionString, psqlPath, additionalFlags, service, cmd) + err := launchPsqlWithConnectionString(connectionString, psqlPath, additionalFlags, service, "tsdbadmin", cmd) // Should fail with exec error since fake psql path doesn't exist if err == nil { @@ -335,11 +335,11 @@ func TestBuildPsqlCommand_KeyringPasswordEnvVar(t *testing.T) { // Store a test password in keyring testPassword := "test-password-12345" storage := password.GetPasswordStorage() - err := storage.Save(service, testPassword) + err := storage.Save(service, testPassword, "tsdbadmin") if err != nil { t.Fatalf("Failed to save test password: %v", err) } - defer storage.Remove(service) // Clean up after test + defer storage.Remove(service, "tsdbadmin") // Clean up after test connectionString := "postgresql://testuser@testhost:5432/testdb?sslmode=require" psqlPath := "/usr/bin/psql" @@ -349,7 +349,7 @@ func TestBuildPsqlCommand_KeyringPasswordEnvVar(t *testing.T) { testCmd := &cobra.Command{} // Call the actual production function that builds the command - psqlCmd := buildPsqlCommand(connectionString, psqlPath, additionalFlags, service, testCmd) + psqlCmd := buildPsqlCommand(connectionString, psqlPath, additionalFlags, service, "tsdbadmin", testCmd) if psqlCmd == nil { t.Fatal("buildPsqlCommand returned nil") @@ -391,7 +391,7 @@ func TestBuildPsqlCommand_PgpassStorage_NoEnvVar(t *testing.T) { testCmd := &cobra.Command{} // Call the actual production function that builds the command - psqlCmd := buildPsqlCommand(connectionString, psqlPath, []string{}, service, testCmd) + psqlCmd := buildPsqlCommand(connectionString, psqlPath, []string{}, service, "tsdbadmin", testCmd) if psqlCmd == nil { t.Fatal("buildPsqlCommand returned nil") @@ -964,11 +964,11 @@ func TestDBConnectionString_WithPassword(t *testing.T) { // Store a test password testPassword := "test-e2e-password-789" storage := password.GetPasswordStorage() - err := storage.Save(service, testPassword) + err := storage.Save(service, testPassword, "tsdbadmin") if err != nil { t.Fatalf("Failed to save test password: %v", err) } - defer storage.Remove(service) // Clean up after test + defer storage.Remove(service, "tsdbadmin") // Clean up after test // Test connection string without password (default behavior) details, err := password.GetConnectionDetails(service, password.ConnectionDetailsOptions{ diff --git a/internal/tiger/cmd/service.go b/internal/tiger/cmd/service.go index fe80d3d8..56715311 100644 --- a/internal/tiger/cmd/service.go +++ b/internal/tiger/cmd/service.go @@ -826,7 +826,7 @@ func waitForServiceReady(client *api.ClientWithResponses, projectID, serviceID s func handlePasswordSaving(service api.Service, initialPassword string, output io.Writer) bool { // Note: We don't fail the service creation if password saving fails // The error is handled by displaying the appropriate message below - result, _ := password.SavePasswordWithResult(service, initialPassword) + result, _ := password.SavePasswordWithResult(service, initialPassword, "tsdbadmin") if result.Method == "none" && result.Message == "No password provided" { // Don't output anything for empty password diff --git a/internal/tiger/mcp/service_tools.go b/internal/tiger/mcp/service_tools.go index 99fbd5b0..aa75de70 100644 --- a/internal/tiger/mcp/service_tools.go +++ b/internal/tiger/mcp/service_tools.go @@ -466,7 +466,7 @@ func (s *Server) handleServiceCreate(ctx context.Context, req *mcp.CallToolReque // Save password immediately after service creation, before any waiting // This ensures the password is stored even if the wait fails or is interrupted if service.InitialPassword != nil { - result, err := password.SavePasswordWithResult(api.Service(service), *service.InitialPassword) + result, err := password.SavePasswordWithResult(api.Service(service), *service.InitialPassword, "tsdbadmin") output.PasswordStorage = &result if err != nil { logging.Debug("MCP: Password storage failed", zap.Error(err)) @@ -566,7 +566,7 @@ func (s *Server) handleServiceUpdatePassword(ctx context.Context, req *mcp.CallT serviceResp, err := apiClient.GetProjectsProjectIdServicesServiceIdWithResponse(ctx, cfg.ProjectID, input.ServiceID) if err == nil && serviceResp.StatusCode() == 200 && serviceResp.JSON200 != nil { // Save the new password using the shared util function - result, err := password.SavePasswordWithResult(api.Service(*serviceResp.JSON200), input.Password) + result, err := password.SavePasswordWithResult(api.Service(*serviceResp.JSON200), input.Password, "tsdbadmin") output.PasswordStorage = &result if err != nil { logging.Debug("MCP: Password storage failed", zap.Error(err)) diff --git a/internal/tiger/password/connection.go b/internal/tiger/password/connection.go index 7ed569cc..e72c0d9f 100644 --- a/internal/tiger/password/connection.go +++ b/internal/tiger/password/connection.go @@ -66,7 +66,7 @@ func GetConnectionDetails(service api.Service, opts ConnectionDetailsOptions) (* if opts.WithPassword { if opts.InitialPassword != "" { details.Password = opts.InitialPassword - } else if password, err := GetPassword(service); err == nil { + } else if password, err := GetPassword(service, opts.Role); err == nil { details.Password = password } } @@ -87,9 +87,9 @@ func (d *ConnectionDetails) String() string { // GetPassword fetches the password for the specified service from the // configured password storage mechanism. It returns an error if it fails to // find the password. -func GetPassword(service api.Service) (string, error) { +func GetPassword(service api.Service, role string) (string, error) { storage := GetPasswordStorage() - password, err := storage.Get(service) + password, err := storage.Get(service, role) if err != nil { // Provide specific error messages based on storage type switch storage.(type) { diff --git a/internal/tiger/password/connection_test.go b/internal/tiger/password/connection_test.go index 318da483..fe542466 100644 --- a/internal/tiger/password/connection_test.go +++ b/internal/tiger/password/connection_test.go @@ -172,12 +172,13 @@ func TestBuildConnectionString_WithPassword_KeyringStorage(t *testing.T) { // Store a test password in keyring testPassword := "test-password-keyring-123" + role := "tsdbadmin" storage := GetPasswordStorage() - err := storage.Save(service, testPassword) + err := storage.Save(service, testPassword, role) if err != nil { t.Fatalf("Failed to save test password: %v", err) } - defer storage.Remove(service) // Clean up after test + defer storage.Remove(service, role) // Clean up after test details, err := GetConnectionDetails(service, ConnectionDetailsOptions{ Role: "tsdbadmin", @@ -223,12 +224,13 @@ func TestBuildConnectionString_WithPassword_PgpassStorage(t *testing.T) { // Store a test password in pgpass testPassword := "test-password-pgpass-456" + role := "tsdbadmin" storage := GetPasswordStorage() - err := storage.Save(service, testPassword) + err := storage.Save(service, testPassword, role) if err != nil { t.Fatalf("Failed to save test password: %v", err) } - defer storage.Remove(service) // Clean up after test + defer storage.Remove(service, role) // Clean up after test details, err := GetConnectionDetails(service, ConnectionDetailsOptions{ Role: "tsdbadmin", diff --git a/internal/tiger/password/password_storage.go b/internal/tiger/password/password_storage.go index 4ee19a89..4b6e1146 100644 --- a/internal/tiger/password/password_storage.go +++ b/internal/tiger/password/password_storage.go @@ -20,15 +20,18 @@ func getPasswordServiceName() string { } // buildPasswordKeyringUsername creates a unique keyring username for service passwords -func buildPasswordKeyringUsername(service api.Service) (string, error) { +func buildPasswordKeyringUsername(service api.Service, role string) (string, error) { if service.ServiceId == nil { return "", fmt.Errorf("service ID is required") } if service.ProjectId == nil { return "", fmt.Errorf("project ID is required") } + if role == "" { + return "", fmt.Errorf("role is required") + } - return fmt.Sprintf("password-%s-%s", *service.ProjectId, *service.ServiceId), nil + return fmt.Sprintf("password-%s-%s-%s", *service.ProjectId, *service.ServiceId, role), nil } // sanitizeErrorMessage removes sensitive information from error messages @@ -53,17 +56,17 @@ type PasswordStorageResult struct { // PasswordStorage defines the interface for password storage implementations type PasswordStorage interface { - Save(service api.Service, password string) error - Get(service api.Service) (string, error) - Remove(service api.Service) error + Save(service api.Service, password string, role string) error + Get(service api.Service, role string) (string, error) + Remove(service api.Service, role string) error GetStorageResult(err error, password string) PasswordStorageResult } // KeyringStorage implements password storage using system keyring type KeyringStorage struct{} -func (k *KeyringStorage) Save(service api.Service, password string) error { - username, err := buildPasswordKeyringUsername(service) +func (k *KeyringStorage) Save(service api.Service, password string, role string) error { + username, err := buildPasswordKeyringUsername(service, role) if err != nil { return err } @@ -71,8 +74,8 @@ func (k *KeyringStorage) Save(service api.Service, password string) error { return keyring.Set(getPasswordServiceName(), username, password) } -func (k *KeyringStorage) Get(service api.Service) (string, error) { - username, err := buildPasswordKeyringUsername(service) +func (k *KeyringStorage) Get(service api.Service, role string) (string, error) { + username, err := buildPasswordKeyringUsername(service, role) if err != nil { return "", err } @@ -80,8 +83,8 @@ func (k *KeyringStorage) Get(service api.Service) (string, error) { return keyring.Get(getPasswordServiceName(), username) } -func (k *KeyringStorage) Remove(service api.Service) error { - username, err := buildPasswordKeyringUsername(service) +func (k *KeyringStorage) Remove(service api.Service, role string) error { + username, err := buildPasswordKeyringUsername(service, role) if err != nil { return err } @@ -108,10 +111,13 @@ func (k *KeyringStorage) GetStorageResult(err error, password string) PasswordSt // PgpassStorage implements password storage using ~/.pgpass file type PgpassStorage struct{} -func (p *PgpassStorage) Save(service api.Service, password string) error { +func (p *PgpassStorage) Save(service api.Service, password string, role string) error { if service.Endpoint == nil || service.Endpoint.Host == nil { return fmt.Errorf("service endpoint not available") } + if role == "" { + return fmt.Errorf("role is required") + } homeDir, err := os.UserHomeDir() if err != nil { @@ -124,8 +130,8 @@ func (p *PgpassStorage) Save(service api.Service, password string) error { if service.Endpoint.Port != nil { port = fmt.Sprintf("%d", *service.Endpoint.Port) } - database := "tsdb" // TimescaleDB database name - username := "tsdbadmin" // default admin user + database := "tsdb" // TimescaleDB database name + username := role // Use the provided role as username // Remove existing entry first (if it exists) if err := p.removeEntry(pgpassPath, host, port, username); err != nil { @@ -149,10 +155,13 @@ func (p *PgpassStorage) Save(service api.Service, password string) error { return nil } -func (p *PgpassStorage) Get(service api.Service) (string, error) { +func (p *PgpassStorage) Get(service api.Service, role string) (string, error) { if service.Endpoint == nil || service.Endpoint.Host == nil { return "", fmt.Errorf("service endpoint not available") } + if role == "" { + return "", fmt.Errorf("role is required") + } homeDir, err := os.UserHomeDir() if err != nil { @@ -171,7 +180,7 @@ func (p *PgpassStorage) Get(service api.Service) (string, error) { if service.Endpoint.Port != nil { port = fmt.Sprintf("%d", *service.Endpoint.Port) } - username := "tsdbadmin" + username := role // Read and parse .pgpass file content, err := os.ReadFile(pgpassPath) @@ -200,10 +209,13 @@ func (p *PgpassStorage) Get(service api.Service) (string, error) { return "", fmt.Errorf("no matching entry found in .pgpass file") } -func (p *PgpassStorage) Remove(service api.Service) error { +func (p *PgpassStorage) Remove(service api.Service, role string) error { if service.Endpoint == nil || service.Endpoint.Host == nil { return fmt.Errorf("service endpoint not available") } + if role == "" { + return fmt.Errorf("role is required") + } homeDir, err := os.UserHomeDir() if err != nil { @@ -216,7 +228,7 @@ func (p *PgpassStorage) Remove(service api.Service) error { if service.Endpoint.Port != nil { port = fmt.Sprintf("%d", *service.Endpoint.Port) } - username := "tsdbadmin" + username := role return p.removeEntry(pgpassPath, host, port, username) } @@ -298,15 +310,15 @@ func (p *PgpassStorage) GetStorageResult(err error, password string) PasswordSto // NoStorage implements no password storage (passwords are not saved) type NoStorage struct{} -func (n *NoStorage) Save(service api.Service, password string) error { +func (n *NoStorage) Save(service api.Service, password string, role string) error { return nil // Do nothing } -func (n *NoStorage) Get(service api.Service) (string, error) { +func (n *NoStorage) Get(service api.Service, role string) (string, error) { return "", fmt.Errorf("password storage disabled") } -func (n *NoStorage) Remove(service api.Service) error { +func (n *NoStorage) Remove(service api.Service, role string) error { return nil // Do nothing } @@ -334,7 +346,7 @@ func GetPasswordStorage() PasswordStorage { } // SavePasswordWithResult handles saving a password and returns both error and result info -func SavePasswordWithResult(service api.Service, password string) (PasswordStorageResult, error) { +func SavePasswordWithResult(service api.Service, password string, role string) (PasswordStorageResult, error) { if password == "" { return PasswordStorageResult{ Success: false, @@ -342,9 +354,16 @@ func SavePasswordWithResult(service api.Service, password string) (PasswordStora Message: "No password provided", }, nil } + if role == "" { + return PasswordStorageResult{ + Success: false, + Method: "none", + Message: "Role is required", + }, fmt.Errorf("role is required") + } storage := GetPasswordStorage() - err := storage.Save(service, password) + err := storage.Save(service, password, role) result := storage.GetStorageResult(err, password) return result, err diff --git a/internal/tiger/password/password_storage_test.go b/internal/tiger/password/password_storage_test.go index 006df586..2060775c 100644 --- a/internal/tiger/password/password_storage_test.go +++ b/internal/tiger/password/password_storage_test.go @@ -30,7 +30,7 @@ func TestNoStorage_Save(t *testing.T) { storage := &NoStorage{} service := createTestService("test-service-123") - err := storage.Save(service, "test-password") + err := storage.Save(service, "test-password", "tsdbadmin") if err != nil { t.Errorf("NoStorage.Save() should never return an error, got: %v", err) } @@ -40,7 +40,7 @@ func TestNoStorage_Get(t *testing.T) { storage := &NoStorage{} service := createTestService("test-service-123") - password, err := storage.Get(service) + password, err := storage.Get(service, "tsdbadmin") if err == nil { t.Error("NoStorage.Get() should return an error") } @@ -56,7 +56,7 @@ func TestNoStorage_Remove(t *testing.T) { storage := &NoStorage{} service := createTestService("test-service-123") - err := storage.Remove(service) + err := storage.Remove(service, "tsdbadmin") if err != nil { t.Errorf("NoStorage.Remove() should never return an error, got: %v", err) } @@ -66,7 +66,7 @@ func TestKeyringStorage_Save_NoServiceId(t *testing.T) { storage := &KeyringStorage{} service := api.Service{} // No ServiceId - err := storage.Save(service, "test-password") + err := storage.Save(service, "test-password", "tsdbadmin") if err == nil { t.Error("KeyringStorage.Save() should return error when ServiceId is nil") } @@ -83,7 +83,7 @@ func TestKeyringStorage_Save_NoProjectId(t *testing.T) { // No ProjectId } - err := storage.Save(service, "test-password") + err := storage.Save(service, "test-password", "tsdbadmin") if err == nil { t.Error("KeyringStorage.Save() should return error when ProjectId is nil") } @@ -92,11 +92,24 @@ func TestKeyringStorage_Save_NoProjectId(t *testing.T) { } } +func TestKeyringStorage_Save_NoRole(t *testing.T) { + storage := &KeyringStorage{} + service := createTestService("test-service-123") + + err := storage.Save(service, "test-password", "") + if err == nil { + t.Error("KeyringStorage.Save() should return error when role is empty") + } + if !strings.Contains(err.Error(), "role is required") { + t.Errorf("KeyringStorage.Save() should mention role required, got: %v", err) + } +} + func TestKeyringStorage_Get_NoServiceId(t *testing.T) { storage := &KeyringStorage{} service := api.Service{} // No ServiceId - password, err := storage.Get(service) + password, err := storage.Get(service, "tsdbadmin") if err == nil { t.Error("KeyringStorage.Get() should return error when ServiceId is nil") } @@ -116,7 +129,7 @@ func TestKeyringStorage_Get_NoProjectId(t *testing.T) { // No ProjectId } - password, err := storage.Get(service) + password, err := storage.Get(service, "tsdbadmin") if err == nil { t.Error("KeyringStorage.Get() should return error when ProjectId is nil") } @@ -128,11 +141,27 @@ func TestKeyringStorage_Get_NoProjectId(t *testing.T) { } } +func TestKeyringStorage_Get_NoRole(t *testing.T) { + storage := &KeyringStorage{} + service := createTestService("test-service-123") + + password, err := storage.Get(service, "") + if err == nil { + t.Error("KeyringStorage.Get() should return error when role is empty") + } + if password != "" { + t.Errorf("KeyringStorage.Get() should return empty password on error, got: %s", password) + } + if !strings.Contains(err.Error(), "role is required") { + t.Errorf("KeyringStorage.Get() should mention role required, got: %v", err) + } +} + func TestKeyringStorage_Remove_NoServiceId(t *testing.T) { storage := &KeyringStorage{} service := api.Service{} // No ServiceId - err := storage.Remove(service) + err := storage.Remove(service, "tsdbadmin") if err == nil { t.Error("KeyringStorage.Remove() should return error when ServiceId is nil") } @@ -149,7 +178,7 @@ func TestKeyringStorage_Remove_NoProjectId(t *testing.T) { // No ProjectId } - err := storage.Remove(service) + err := storage.Remove(service, "tsdbadmin") if err == nil { t.Error("KeyringStorage.Remove() should return error when ProjectId is nil") } @@ -158,6 +187,19 @@ func TestKeyringStorage_Remove_NoProjectId(t *testing.T) { } } +func TestKeyringStorage_Remove_NoRole(t *testing.T) { + storage := &KeyringStorage{} + service := createTestService("test-service-123") + + err := storage.Remove(service, "") + if err == nil { + t.Error("KeyringStorage.Remove() should return error when role is empty") + } + if !strings.Contains(err.Error(), "role is required") { + t.Errorf("KeyringStorage.Remove() should mention role required, got: %v", err) + } +} + func TestPgpassStorage_Remove_NoEndpoint(t *testing.T) { storage := &PgpassStorage{} service := api.Service{ @@ -165,7 +207,7 @@ func TestPgpassStorage_Remove_NoEndpoint(t *testing.T) { // No Endpoint } - err := storage.Remove(service) + err := storage.Remove(service, "tsdbadmin") if err == nil { t.Error("PgpassStorage.Remove() should return error when endpoint is nil") } @@ -174,6 +216,19 @@ func TestPgpassStorage_Remove_NoEndpoint(t *testing.T) { } } +func TestPgpassStorage_Remove_NoRole(t *testing.T) { + storage := &PgpassStorage{} + service := createTestService("test-service-123") + + err := storage.Remove(service, "") + if err == nil { + t.Error("PgpassStorage.Remove() should return error when role is empty") + } + if !strings.Contains(err.Error(), "role is required") { + t.Errorf("PgpassStorage.Remove() should mention role required, got: %v", err) + } +} + func TestPgpassStorage_Get_NoEndpoint(t *testing.T) { storage := &PgpassStorage{} service := api.Service{ @@ -181,7 +236,7 @@ func TestPgpassStorage_Get_NoEndpoint(t *testing.T) { // No Endpoint } - password, err := storage.Get(service) + password, err := storage.Get(service, "tsdbadmin") if err == nil { t.Error("PgpassStorage.Get() should return error when endpoint is nil") } @@ -193,6 +248,22 @@ func TestPgpassStorage_Get_NoEndpoint(t *testing.T) { } } +func TestPgpassStorage_Get_NoRole(t *testing.T) { + storage := &PgpassStorage{} + service := createTestService("test-service-123") + + password, err := storage.Get(service, "") + if err == nil { + t.Error("PgpassStorage.Get() should return error when role is empty") + } + if password != "" { + t.Errorf("PgpassStorage.Get() should return empty password on error, got: %s", password) + } + if !strings.Contains(err.Error(), "role is required") { + t.Errorf("PgpassStorage.Get() should mention role required, got: %v", err) + } +} + func TestPgpassStorage_Get_NoFile(t *testing.T) { // Create a temporary directory with no .pgpass file tempDir := t.TempDir() @@ -203,7 +274,7 @@ func TestPgpassStorage_Get_NoFile(t *testing.T) { storage := &PgpassStorage{} service := createTestService("test-service-123") - password, err := storage.Get(service) + password, err := storage.Get(service, "tsdbadmin") if err == nil { t.Error("PgpassStorage.Get() should return error when .pgpass file doesn't exist") } @@ -246,6 +317,123 @@ func TestGetPasswordStorage(t *testing.T) { } } +// Test password storage with different roles +func TestPasswordStorage_DifferentRoles(t *testing.T) { + tests := []struct { + name string + storage PasswordStorage + setup func(t *testing.T) + cleanup func(t *testing.T, service api.Service, roles []string) + }{ + { + name: "KeyringStorage", + storage: &KeyringStorage{}, + setup: func(t *testing.T) { + config.SetTestServiceName(t) + }, + cleanup: func(t *testing.T, service api.Service, roles []string) { + storage := &KeyringStorage{} + for _, role := range roles { + storage.Remove(service, role) + } + }, + }, + { + name: "PgpassStorage", + storage: &PgpassStorage{}, + setup: func(t *testing.T) { + tempDir := t.TempDir() + originalHome := os.Getenv("HOME") + os.Setenv("HOME", tempDir) + t.Cleanup(func() { + os.Setenv("HOME", originalHome) + }) + }, + }, + } + + roles := []struct { + role string + password string + }{ + {"tsdbadmin", "admin-password-123"}, + {"readonly", "readonly-password-456"}, + {"app_user", "app-password-789"}, + {"MyAppUser", "mixedcase-password-101"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.setup != nil { + tt.setup(t) + } + + service := createTestService("role-test-service") + roleNames := make([]string, len(roles)) + for i, r := range roles { + roleNames[i] = r.role + } + + if tt.cleanup != nil { + defer tt.cleanup(t, service, roleNames) + } + + // Save passwords for all roles + for _, r := range roles { + err := tt.storage.Save(service, r.password, r.role) + if _, ok := tt.storage.(*NoStorage); ok { + continue + } + if err != nil { + t.Skipf("Storage not available: %v", err) + } + } + + // Verify each role has its own password stored separately + for _, r := range roles { + retrieved, err := tt.storage.Get(service, r.role) + if _, ok := tt.storage.(*NoStorage); ok { + if err == nil { + t.Error("NoStorage.Get() should always return error") + } + continue + } + if err != nil { + t.Fatalf("Get(%s) failed: %v", r.role, err) + } + if retrieved != r.password { + t.Errorf("Get(%s) = %q, want %q", r.role, retrieved, r.password) + } + } + + // Verify removing one role doesn't affect others + if _, ok := tt.storage.(*NoStorage); !ok { + err := tt.storage.Remove(service, roles[0].role) + if err != nil { + t.Fatalf("Remove(%s) failed: %v", roles[0].role, err) + } + + // First role should be gone + _, err = tt.storage.Get(service, roles[0].role) + if err == nil { + t.Errorf("Get(%s) should fail after removal", roles[0].role) + } + + // Other roles should still exist + for _, r := range roles[1:] { + retrieved, err := tt.storage.Get(service, r.role) + if err != nil { + t.Errorf("Get(%s) should still work after removing %s: %v", r.role, roles[0].role, err) + } + if retrieved != r.password { + t.Errorf("Get(%s) = %q, want %q", r.role, retrieved, r.password) + } + } + } + }) + } +} + // Test pgpass storage with a temporary directory func TestPgpassStorage_Integration(t *testing.T) { // Create a temporary directory for this test @@ -259,9 +447,10 @@ func TestPgpassStorage_Integration(t *testing.T) { storage := &PgpassStorage{} service := createTestService("test-service-123") password := "test-password" + role := "tsdbadmin" // Test Save - err := storage.Save(service, password) + err := storage.Save(service, password, role) if err != nil { t.Fatalf("PgpassStorage.Save() failed: %v", err) } @@ -284,7 +473,7 @@ func TestPgpassStorage_Integration(t *testing.T) { } // Test Get - retrievedPassword, err := storage.Get(service) + retrievedPassword, err := storage.Get(service, role) if err != nil { t.Fatalf("PgpassStorage.Get() failed: %v", err) } @@ -293,7 +482,7 @@ func TestPgpassStorage_Integration(t *testing.T) { } // Test Remove - err = storage.Remove(service) + err = storage.Remove(service, role) if err != nil { t.Fatalf("PgpassStorage.Remove() failed: %v", err) } @@ -431,7 +620,7 @@ func TestPgpassStorage_GetStorageResult_Error(t *testing.T) { func TestSavePasswordWithResult_EmptyPassword(t *testing.T) { service := createTestService("test-service-123") - result, err := SavePasswordWithResult(service, "") + result, err := SavePasswordWithResult(service, "", "tsdbadmin") if err != nil { t.Errorf("SavePasswordWithResult() with empty password should not return error, got: %v", err) } @@ -453,7 +642,7 @@ func TestSavePasswordWithResult_WithPassword(t *testing.T) { viper.Set("password_storage", "none") defer viper.Set("password_storage", "") - result, err := SavePasswordWithResult(service, "test-password") + result, err := SavePasswordWithResult(service, "test-password", "tsdbadmin") if err != nil { t.Errorf("SavePasswordWithResult() should not return error with NoStorage, got: %v", err) } @@ -510,16 +699,38 @@ func TestBuildPasswordKeyringUsername(t *testing.T) { tests := []struct { name string service api.Service + role string expected string expectError bool }{ { - name: "valid service with both IDs", + name: "valid service with both IDs and tsdbadmin role", service: api.Service{ ProjectId: util.Ptr("project-123"), ServiceId: util.Ptr("service-456"), }, - expected: "password-project-123-service-456", + role: "tsdbadmin", + expected: "password-project-123-service-456-tsdbadmin", + expectError: false, + }, + { + name: "valid service with both IDs and readonly role", + service: api.Service{ + ProjectId: util.Ptr("project-123"), + ServiceId: util.Ptr("service-456"), + }, + role: "readonly", + expected: "password-project-123-service-456-readonly", + expectError: false, + }, + { + name: "valid service with both IDs and mixed-case role", + service: api.Service{ + ProjectId: util.Ptr("project-123"), + ServiceId: util.Ptr("service-456"), + }, + role: "MyAppUser", + expected: "password-project-123-service-456-MyAppUser", expectError: false, }, { @@ -527,6 +738,7 @@ func TestBuildPasswordKeyringUsername(t *testing.T) { service: api.Service{ ProjectId: util.Ptr("project-123"), }, + role: "tsdbadmin", expected: "", expectError: true, }, @@ -535,12 +747,24 @@ func TestBuildPasswordKeyringUsername(t *testing.T) { service: api.Service{ ServiceId: util.Ptr("service-456"), }, + role: "tsdbadmin", expected: "", expectError: true, }, { name: "missing both IDs", service: api.Service{}, + role: "tsdbadmin", + expected: "", + expectError: true, + }, + { + name: "missing role", + service: api.Service{ + ProjectId: util.Ptr("project-123"), + ServiceId: util.Ptr("service-456"), + }, + role: "", expected: "", expectError: true, }, @@ -548,7 +772,7 @@ func TestBuildPasswordKeyringUsername(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := buildPasswordKeyringUsername(tt.service) + result, err := buildPasswordKeyringUsername(tt.service, tt.role) if tt.expectError && err == nil { t.Errorf("buildPasswordKeyringUsername() expected error but got none") @@ -580,7 +804,7 @@ func TestPasswordStorage_OverwritePreviousValue(t *testing.T) { }, cleanup: func(t *testing.T, service api.Service) { storage := &KeyringStorage{} - storage.Remove(service) // Ignore errors in cleanup + storage.Remove(service, "tsdbadmin") // Ignore errors in cleanup }, }, { @@ -615,12 +839,13 @@ func TestPasswordStorage_OverwritePreviousValue(t *testing.T) { originalPassword := "original-password-123" newPassword := "new-password-456" + role := "tsdbadmin" // Save original password - err1 := tt.storage.Save(service, originalPassword) + err1 := tt.storage.Save(service, originalPassword, role) // Save new password (should overwrite) - err2 := tt.storage.Save(service, newPassword) + err2 := tt.storage.Save(service, newPassword, role) // Handle NoStorage specially (always succeeds but doesn't store) if _, ok := tt.storage.(*NoStorage); ok { @@ -628,7 +853,7 @@ func TestPasswordStorage_OverwritePreviousValue(t *testing.T) { t.Errorf("NoStorage.Save() should not return error, got first: %v, second: %v", err1, err2) } // NoStorage.Get() always returns error, so we can't test retrieval - _, err := tt.storage.Get(service) + _, err := tt.storage.Get(service, role) if err == nil { t.Error("NoStorage.Get() should always return error") } @@ -648,7 +873,7 @@ func TestPasswordStorage_OverwritePreviousValue(t *testing.T) { } // Get the stored password - should be the new one - retrieved, err := tt.storage.Get(service) + retrieved, err := tt.storage.Get(service, role) if err != nil { t.Fatalf("Get() failed: %v", err) } From 33e4f00d47771d1026a789e5f0108e7dfe9a024a Mon Sep 17 00:00:00 2001 From: Matvey Arye Date: Wed, 15 Oct 2025 13:36:07 +0200 Subject: [PATCH 3/4] Implement db save-password command with role support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add new command to save database passwords with support for different database roles. Passwords can be provided via three methods: 1. --password=value flag (highest precedence) 2. TIGER_NEW_PASSWORD environment variable 3. Interactive prompt (when neither provided) The command stores passwords according to the --password-storage setting (keyring, pgpass, or none) and supports saving passwords for different database roles independently. Features: - Save passwords for any database role (default: tsdbadmin) - Three input methods with clear precedence - Integration with existing password storage system - Comprehensive test coverage for all input methods Example usage: tiger db save-password svc-12345 --password=mypass tiger db save-password svc-12345 --role readonly tiger db save-password svc-12345 # prompts interactively 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/tiger/cmd/db.go | 97 +++++++ internal/tiger/cmd/db_test.go | 475 ++++++++++++++++++++++++++++++++++ specs/spec.md | 20 +- 3 files changed, 582 insertions(+), 10 deletions(-) diff --git a/internal/tiger/cmd/db.go b/internal/tiger/cmd/db.go index 8527de81..9c68f8a9 100644 --- a/internal/tiger/cmd/db.go +++ b/internal/tiger/cmd/db.go @@ -1,6 +1,7 @@ package cmd import ( + "bufio" "context" "errors" "fmt" @@ -20,6 +21,9 @@ import ( var ( // getAPIKeyForDB can be overridden for testing getAPIKeyForDB = config.GetAPIKey + + // getServiceDetailsFunc can be overridden for testing + getServiceDetailsFunc = getServiceDetails ) func buildDbConnectionStringCmd() *cobra.Command { @@ -241,6 +245,98 @@ Examples: return cmd } +func buildDbSavePasswordCmd() *cobra.Command { + var dbSavePasswordRole string + var dbSavePasswordValue string + + cmd := &cobra.Command{ + Use: "save-password [service-id]", + Short: "Save password for a database service", + Long: `Save a password for a database service to configured password storage. + +The service ID can be provided as an argument or will use the default service +from your configuration. The password can be provided via: +1. --password flag with explicit value (highest precedence) +2. TIGER_NEW_PASSWORD environment variable +3. Interactive prompt (if neither provided) + +The password will be saved according to your --password-storage setting +(keyring, pgpass, or none). + +Examples: + # Save password with explicit value (highest precedence) + tiger db save-password svc-12345 --password=your-password + + # Using environment variable + export TIGER_NEW_PASSWORD=your-password + tiger db save-password svc-12345 + + # Interactive password prompt (when neither flag nor env var provided) + tiger db save-password svc-12345 + + # Save password for custom role + tiger db save-password svc-12345 --password=your-password --role readonly + + # Save to specific storage location + tiger db save-password svc-12345 --password=your-password --password-storage pgpass`, + RunE: func(cmd *cobra.Command, args []string) error { + service, err := getServiceDetailsFunc(cmd, args) + if err != nil { + return err + } + + // Determine password based on precedence: + // 1. --password flag with value + // 2. TIGER_NEW_PASSWORD environment variable + // 3. Interactive prompt + var passwordToSave string + + if cmd.Flags().Changed("password") { + // --password flag was provided + passwordToSave = dbSavePasswordValue + if passwordToSave == "" { + return fmt.Errorf("password cannot be empty when provided via --password flag") + } + } else if envPassword := os.Getenv("TIGER_NEW_PASSWORD"); envPassword != "" { + // Use environment variable + passwordToSave = envPassword + } else { + // Interactive prompt + fmt.Fprint(cmd.OutOrStdout(), "Enter password: ") + scanner := bufio.NewScanner(cmd.InOrStdin()) + if scanner.Scan() { + passwordToSave = scanner.Text() + } else { + if err := scanner.Err(); err != nil { + return fmt.Errorf("failed to read password: %w", err) + } + return fmt.Errorf("failed to read password: EOF") + } + if passwordToSave == "" { + return fmt.Errorf("password cannot be empty") + } + } + + // Save password using configured storage + storage := password.GetPasswordStorage() + err = storage.Save(service, passwordToSave, dbSavePasswordRole) + if err != nil { + return fmt.Errorf("failed to save password: %w", err) + } + + fmt.Fprintf(cmd.OutOrStdout(), "Password saved successfully for service %s (role: %s)\n", + *service.ServiceId, dbSavePasswordRole) + return nil + }, + } + + // Add flags for db save-password command + cmd.Flags().StringVar(&dbSavePasswordValue, "password", "", "Password to save") + cmd.Flags().StringVar(&dbSavePasswordRole, "role", "tsdbadmin", "Database role/username") + + return cmd +} + func buildDbCmd() *cobra.Command { cmd := &cobra.Command{ Use: "db", @@ -251,6 +347,7 @@ func buildDbCmd() *cobra.Command { cmd.AddCommand(buildDbConnectionStringCmd()) cmd.AddCommand(buildDbConnectCmd()) cmd.AddCommand(buildDbTestConnectionCmd()) + cmd.AddCommand(buildDbSavePasswordCmd()) return cmd } diff --git a/internal/tiger/cmd/db_test.go b/internal/tiger/cmd/db_test.go index 5d5bd0a4..1d65849c 100644 --- a/internal/tiger/cmd/db_test.go +++ b/internal/tiger/cmd/db_test.go @@ -1009,3 +1009,478 @@ func TestDBConnectionString_WithPassword(t *testing.T) { t.Errorf("Connection string with password should contain '%s', but it doesn't: %s", testPassword, connectionStringWithPassword) } } + +func TestDBSavePassword_ExplicitPassword(t *testing.T) { + // Use a unique service name for this test to avoid conflicts + config.SetTestServiceName(t) + tmpDir := setupDBTest(t) + + // Set keyring as the password storage method for this test + originalStorage := viper.GetString("password_storage") + viper.Set("password_storage", "keyring") + defer viper.Set("password_storage", originalStorage) + + // Set up config + _, err := config.UseTestConfig(tmpDir, map[string]any{ + "api_url": "http://localhost:9999", + "project_id": "test-project-123", + "service_id": "svc-save-test", + }) + if err != nil { + t.Fatalf("Failed to save test config: %v", err) + } + + // Mock getServiceDetailsFunc to return a test service + serviceID := "svc-save-test" + projectID := "test-project-123" + host := "test-host.com" + port := 5432 + mockService := api.Service{ + ServiceId: &serviceID, + ProjectId: &projectID, + Endpoint: &api.Endpoint{ + Host: &host, + Port: &port, + }, + } + + originalGetServiceDetails := getServiceDetailsFunc + getServiceDetailsFunc = func(cmd *cobra.Command, args []string) (api.Service, error) { + return mockService, nil + } + defer func() { getServiceDetailsFunc = originalGetServiceDetails }() + + testPassword := "explicit-password-123" + + // Execute save-password with explicit password + output, err := executeDBCommand("db", "save-password", "--password="+testPassword) + if err != nil { + t.Fatalf("Expected save-password to succeed, got error: %v", err) + } + + // Verify success message + if !strings.Contains(output, "Password saved successfully") { + t.Errorf("Expected success message, got: %s", output) + } + if !strings.Contains(output, serviceID) { + t.Errorf("Expected service ID in output, got: %s", output) + } + + // Verify password was actually saved + storage := password.GetPasswordStorage() + retrievedPassword, err := storage.Get(mockService, "tsdbadmin") + if err != nil { + t.Fatalf("Failed to retrieve saved password: %v", err) + } + defer storage.Remove(mockService, "tsdbadmin") + + if retrievedPassword != testPassword { + t.Errorf("Expected password %q, got %q", testPassword, retrievedPassword) + } +} + +func TestDBSavePassword_EnvironmentVariable(t *testing.T) { + // Use a unique service name for this test to avoid conflicts + config.SetTestServiceName(t) + tmpDir := setupDBTest(t) + + // Set keyring as the password storage method for this test + originalStorage := viper.GetString("password_storage") + viper.Set("password_storage", "keyring") + defer viper.Set("password_storage", originalStorage) + + // Set up config + _, err := config.UseTestConfig(tmpDir, map[string]any{ + "api_url": "http://localhost:9999", + "project_id": "test-project-123", + "service_id": "svc-env-test", + }) + if err != nil { + t.Fatalf("Failed to save test config: %v", err) + } + + // Mock getServiceDetailsFunc to return a test service + serviceID := "svc-env-test" + projectID := "test-project-123" + host := "test-host.com" + port := 5432 + mockService := api.Service{ + ServiceId: &serviceID, + ProjectId: &projectID, + Endpoint: &api.Endpoint{ + Host: &host, + Port: &port, + }, + } + + originalGetServiceDetails := getServiceDetailsFunc + getServiceDetailsFunc = func(cmd *cobra.Command, args []string) (api.Service, error) { + return mockService, nil + } + defer func() { getServiceDetailsFunc = originalGetServiceDetails }() + + // Set environment variable + testPassword := "env-password-456" + os.Setenv("TIGER_NEW_PASSWORD", testPassword) + defer os.Unsetenv("TIGER_NEW_PASSWORD") + + // Execute save-password without --password flag (should use env var) + output, err := executeDBCommand("db", "save-password") + if err != nil { + t.Fatalf("Expected save-password to succeed with env var, got error: %v", err) + } + + // Verify success message + if !strings.Contains(output, "Password saved successfully") { + t.Errorf("Expected success message, got: %s", output) + } + + // Verify password was actually saved + storage := password.GetPasswordStorage() + retrievedPassword, err := storage.Get(mockService, "tsdbadmin") + if err != nil { + t.Fatalf("Failed to retrieve saved password: %v", err) + } + defer storage.Remove(mockService, "tsdbadmin") + + if retrievedPassword != testPassword { + t.Errorf("Expected password %q, got %q", testPassword, retrievedPassword) + } +} + +func TestDBSavePassword_InteractivePrompt(t *testing.T) { + // Use a unique service name for this test to avoid conflicts + config.SetTestServiceName(t) + tmpDir := setupDBTest(t) + + // Set keyring as the password storage method for this test + originalStorage := viper.GetString("password_storage") + viper.Set("password_storage", "keyring") + defer viper.Set("password_storage", originalStorage) + + // Set up config + _, err := config.UseTestConfig(tmpDir, map[string]any{ + "api_url": "http://localhost:9999", + "project_id": "test-project-123", + "service_id": "svc-interactive-test", + }) + if err != nil { + t.Fatalf("Failed to save test config: %v", err) + } + + // Mock getServiceDetailsFunc to return a test service + serviceID := "svc-interactive-test" + projectID := "test-project-123" + host := "test-host.com" + port := 5432 + mockService := api.Service{ + ServiceId: &serviceID, + ProjectId: &projectID, + Endpoint: &api.Endpoint{ + Host: &host, + Port: &port, + }, + } + + originalGetServiceDetails := getServiceDetailsFunc + getServiceDetailsFunc = func(cmd *cobra.Command, args []string) (api.Service, error) { + return mockService, nil + } + defer func() { getServiceDetailsFunc = originalGetServiceDetails }() + + // Make sure TIGER_NEW_PASSWORD is not set + os.Unsetenv("TIGER_NEW_PASSWORD") + + // Prepare the password input + testPassword := "interactive-password-999" + inputReader := strings.NewReader(testPassword + "\n") + + // Build the command with custom stdin + testRoot := buildRootCmd() + buf := new(bytes.Buffer) + testRoot.SetOut(buf) + testRoot.SetErr(buf) + testRoot.SetIn(inputReader) // Use our reader as stdin + testRoot.SetArgs([]string{"db", "save-password"}) // No --password flag and no env var + + // Execute the command + err = testRoot.Execute() + if err != nil { + t.Fatalf("Expected save-password to succeed with interactive input, got error: %v", err) + } + + output := buf.String() + + // Verify the prompt was shown + if !strings.Contains(output, "Enter password:") { + t.Errorf("Expected password prompt, got: %s", output) + } + + // Verify success message + if !strings.Contains(output, "Password saved successfully") { + t.Errorf("Expected success message, got: %s", output) + } + + // Verify password was actually saved + storage := password.GetPasswordStorage() + retrievedPassword, err := storage.Get(mockService, "tsdbadmin") + if err != nil { + t.Fatalf("Failed to retrieve saved password: %v", err) + } + defer storage.Remove(mockService, "tsdbadmin") + + if retrievedPassword != testPassword { + t.Errorf("Expected password %q, got %q", testPassword, retrievedPassword) + } +} + +func TestDBSavePassword_InteractivePromptEmpty(t *testing.T) { + tmpDir := setupDBTest(t) + + // Set up config + _, err := config.UseTestConfig(tmpDir, map[string]any{ + "api_url": "http://localhost:9999", + "project_id": "test-project-123", + "service_id": "svc-empty-test", + }) + if err != nil { + t.Fatalf("Failed to save test config: %v", err) + } + + // Mock getServiceDetailsFunc to return a test service + serviceID := "svc-empty-test" + projectID := "test-project-123" + mockService := api.Service{ + ServiceId: &serviceID, + ProjectId: &projectID, + } + + originalGetServiceDetails := getServiceDetailsFunc + getServiceDetailsFunc = func(cmd *cobra.Command, args []string) (api.Service, error) { + return mockService, nil + } + defer func() { getServiceDetailsFunc = originalGetServiceDetails }() + + // Make sure TIGER_NEW_PASSWORD is not set + os.Unsetenv("TIGER_NEW_PASSWORD") + + // Prepare empty input (just a newline) + inputReader := strings.NewReader("\n") + + // Build the command with custom stdin + testRoot := buildRootCmd() + buf := new(bytes.Buffer) + testRoot.SetOut(buf) + testRoot.SetErr(buf) + testRoot.SetIn(inputReader) // Use our reader as stdin + testRoot.SetArgs([]string{"db", "save-password"}) // No --password flag or env var + + // Execute the command + err = testRoot.Execute() + if err == nil { + t.Fatal("Expected error when user provides empty password interactively") + } + + // Verify the error message + if !strings.Contains(err.Error(), "password cannot be empty") { + t.Errorf("Expected 'password cannot be empty' error, got: %v", err) + } +} + +func TestDBSavePassword_CustomRole(t *testing.T) { + // Use a unique service name for this test to avoid conflicts + config.SetTestServiceName(t) + tmpDir := setupDBTest(t) + + // Set keyring as the password storage method for this test + originalStorage := viper.GetString("password_storage") + viper.Set("password_storage", "keyring") + defer viper.Set("password_storage", originalStorage) + + // Set up config + _, err := config.UseTestConfig(tmpDir, map[string]any{ + "api_url": "http://localhost:9999", + "project_id": "test-project-123", + "service_id": "svc-role-test", + }) + if err != nil { + t.Fatalf("Failed to save test config: %v", err) + } + + // Mock getServiceDetailsFunc to return a test service + serviceID := "svc-role-test" + projectID := "test-project-123" + host := "test-host.com" + port := 5432 + mockService := api.Service{ + ServiceId: &serviceID, + ProjectId: &projectID, + Endpoint: &api.Endpoint{ + Host: &host, + Port: &port, + }, + } + + originalGetServiceDetails := getServiceDetailsFunc + getServiceDetailsFunc = func(cmd *cobra.Command, args []string) (api.Service, error) { + return mockService, nil + } + defer func() { getServiceDetailsFunc = originalGetServiceDetails }() + + testPassword := "readonly-password-789" + customRole := "readonly" + + // Execute with custom role + output, err := executeDBCommand("db", "save-password", "--password="+testPassword, "--role", customRole) + if err != nil { + t.Fatalf("Expected save-password to succeed with custom role, got error: %v", err) + } + + // Verify success message shows the custom role + if !strings.Contains(output, "Password saved successfully") { + t.Errorf("Expected success message, got: %s", output) + } + if !strings.Contains(output, customRole) { + t.Errorf("Expected role %q in output, got: %s", customRole, output) + } + + // Verify password was saved for the custom role + storage := password.GetPasswordStorage() + retrievedPassword, err := storage.Get(mockService, customRole) + if err != nil { + t.Fatalf("Failed to retrieve saved password for role %s: %v", customRole, err) + } + defer storage.Remove(mockService, customRole) + + if retrievedPassword != testPassword { + t.Errorf("Expected password %q, got %q", testPassword, retrievedPassword) + } + + // Verify that tsdbadmin role doesn't have this password + _, err = storage.Get(mockService, "tsdbadmin") + if err == nil { + t.Error("Expected error when retrieving password for different role, but got none") + } +} + +func TestDBSavePassword_NoServiceID(t *testing.T) { + tmpDir := setupDBTest(t) + + // Set up config with project ID but no default service ID + _, err := config.UseTestConfig(tmpDir, map[string]any{ + "api_url": "https://api.tigerdata.com/public/v1", + "project_id": "test-project-123", + }) + if err != nil { + t.Fatalf("Failed to save test config: %v", err) + } + + // No need to mock since it should fail before reaching getServiceDetailsFunc + + // Execute save-password without service ID + _, err = executeDBCommand("db", "save-password", "--password=test-password") + if err == nil { + t.Fatal("Expected error when no service ID is provided or configured") + } + + if !strings.Contains(err.Error(), "service ID is required") { + t.Errorf("Expected error about missing service ID, got: %v", err) + } +} + +func TestDBSavePassword_NoAuth(t *testing.T) { + tmpDir := setupDBTest(t) + + // Set up config with project ID and service ID + _, err := config.UseTestConfig(tmpDir, map[string]any{ + "api_url": "https://api.tigerdata.com/public/v1", + "project_id": "test-project-123", + "service_id": "svc-12345", + }) + if err != nil { + t.Fatalf("Failed to save test config: %v", err) + } + + // Mock authentication failure + originalGetAPIKey := getAPIKeyForDB + getAPIKeyForDB = func() (string, error) { + return "", fmt.Errorf("not logged in") + } + defer func() { getAPIKeyForDB = originalGetAPIKey }() + + // Execute save-password command + _, err = executeDBCommand("db", "save-password", "--password=test-password") + if err == nil { + t.Fatal("Expected error when not authenticated") + } + + if !strings.Contains(err.Error(), "authentication required") { + t.Errorf("Expected authentication error, got: %v", err) + } +} + +func TestDBSavePassword_PgpassStorage(t *testing.T) { + // Use a unique service name for this test to avoid conflicts + config.SetTestServiceName(t) + tmpDir := setupDBTest(t) + + // Set pgpass as the password storage method for this test + originalStorage := viper.GetString("password_storage") + viper.Set("password_storage", "pgpass") + defer viper.Set("password_storage", originalStorage) + + // Set up config + _, err := config.UseTestConfig(tmpDir, map[string]any{ + "api_url": "http://localhost:9999", + "project_id": "test-project-123", + "service_id": "svc-pgpass-test", + }) + if err != nil { + t.Fatalf("Failed to save test config: %v", err) + } + + // Mock getServiceDetailsFunc to return a test service with endpoint (required for pgpass) + serviceID := "svc-pgpass-test" + projectID := "test-project-123" + host := "pgpass-host.com" + port := 5432 + mockService := api.Service{ + ServiceId: &serviceID, + ProjectId: &projectID, + Endpoint: &api.Endpoint{ + Host: &host, + Port: &port, + }, + } + + originalGetServiceDetails := getServiceDetailsFunc + getServiceDetailsFunc = func(cmd *cobra.Command, args []string) (api.Service, error) { + return mockService, nil + } + defer func() { getServiceDetailsFunc = originalGetServiceDetails }() + + testPassword := "pgpass-password-101" + + // Execute with pgpass storage + output, err := executeDBCommand("db", "save-password", "--password="+testPassword) + if err != nil { + t.Fatalf("Expected save-password to succeed with pgpass, got error: %v", err) + } + + // Verify success message + if !strings.Contains(output, "Password saved successfully") { + t.Errorf("Expected success message, got: %s", output) + } + + // Verify password was saved in pgpass storage + storage := password.GetPasswordStorage() + retrievedPassword, err := storage.Get(mockService, "tsdbadmin") + if err != nil { + t.Fatalf("Failed to retrieve saved password from pgpass: %v", err) + } + defer storage.Remove(mockService, "tsdbadmin") + + if retrievedPassword != testPassword { + t.Errorf("Expected password %q, got %q", testPassword, retrievedPassword) + } +} diff --git a/specs/spec.md b/specs/spec.md index 405543e6..934a6667 100644 --- a/specs/spec.md +++ b/specs/spec.md @@ -31,7 +31,7 @@ mv tiger /usr/local/bin/ - `TIGER_CONFIG_DIR`: Configuration directory (default: ~/.config/tiger) - `TIGER_OUTPUT`: Default output format (json, yaml, table) - `TIGER_ANALYTICS`: Enable/disable usage analytics (true/false) -- `TIGER_NEW_PASSWORD`: Password to use for save-password command (overridden by --password flag) +- `TIGER_NEW_PASSWORD`: Password to use for save-password command (used when --password flag not provided) ### Configuration File @@ -398,19 +398,19 @@ tiger db test-connection svc-12345 tiger db test-connection svc-12345 --timeout 10s # Save password with explicit value (highest precedence) -tiger db save-password svc-12345 --password your-password -tiger db save-password svc-12345 --password your-password --role readonly +tiger db save-password svc-12345 --password=your-password +tiger db save-password svc-12345 --password=your-password --role readonly -# Interactive password prompt (when --password flag provided with no value) -tiger db save-password svc-12345 --password - -# Using environment variable (only when --password flag not provided) +# Using environment variable export TIGER_NEW_PASSWORD=your-password tiger db save-password svc-12345 +# Interactive password prompt (when neither flag nor env var provided) +tiger db save-password svc-12345 + # Save to specific storage location -tiger db save-password svc-12345 --password your-password --password-storage pgpass -tiger db save-password svc-12345 --password your-password --password-storage keyring +tiger db save-password svc-12345 --password=your-password --password-storage pgpass +tiger db save-password svc-12345 --password=your-password --password-storage keyring # Remove password from configured storage tiger db remove-password svc-12345 @@ -436,7 +436,7 @@ The `connect` and `psql` commands support passing additional flags directly to t **Options:** - `--pooled`: Use connection pooling (for connection-string command) - `--role`: Database role to use (default: tsdbadmin) -- `--password`: Password to save (for save-password command). When flag is provided with no value, prompts interactively. When flag is not provided at all, uses TIGER_NEW_PASSWORD environment variable if set. +- `--password`: Password to save (for save-password command). Provide value with `--password=value`. If flag is not provided, uses TIGER_NEW_PASSWORD environment variable if set, otherwise prompts interactively. - `-t, --timeout`: Timeout for test-connection (accepts any duration format from Go's time.ParseDuration: "3s", "30s", "1m", etc.) (default: 3s, set to 0 to disable) ### High-Availability Management From 221ce149482feee6fd2a4e26d1a58439b68743c3 Mon Sep 17 00:00:00 2001 From: Matvey Arye Date: Sat, 18 Oct 2025 20:37:53 +0200 Subject: [PATCH 4/4] Address PR review feedback for db save-password MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add -p short flag for --password option - Use term.ReadPassword instead of bufio.Scanner for secure password input - Add TTY check before interactive prompt to fail gracefully in non-terminal environments - Scope err variable to conditional block for cleaner code - Output status messages to stderr instead of stdout for consistency - Make TTY check and password reading testable via function variables - Update tests to mock terminal behavior without requiring actual TTY - Move TIGER_NEW_PASSWORD documentation from global to save-password section 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/tiger/cmd/db.go | 39 +++++++++++++++++---------- internal/tiger/cmd/db_test.go | 50 ++++++++++++++++++++--------------- specs/spec.md | 7 ++++- 3 files changed, 59 insertions(+), 37 deletions(-) diff --git a/internal/tiger/cmd/db.go b/internal/tiger/cmd/db.go index 9c68f8a9..0404066e 100644 --- a/internal/tiger/cmd/db.go +++ b/internal/tiger/cmd/db.go @@ -1,7 +1,6 @@ package cmd import ( - "bufio" "context" "errors" "fmt" @@ -12,10 +11,12 @@ import ( "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" "github.com/spf13/cobra" + "golang.org/x/term" "github.com/timescale/tiger-cli/internal/tiger/api" "github.com/timescale/tiger-cli/internal/tiger/config" "github.com/timescale/tiger-cli/internal/tiger/password" + "github.com/timescale/tiger-cli/internal/tiger/util" ) var ( @@ -24,6 +25,16 @@ var ( // getServiceDetailsFunc can be overridden for testing getServiceDetailsFunc = getServiceDetails + + // checkStdinIsTTY can be overridden for testing to bypass TTY detection + checkStdinIsTTY = func() bool { + return util.IsTerminal(os.Stdin) + } + + // readPasswordFromTerminal can be overridden for testing to inject password input + readPasswordFromTerminal = func(fd int) ([]byte, error) { + return term.ReadPassword(fd) + } ) func buildDbConnectionStringCmd() *cobra.Command { @@ -301,17 +312,18 @@ Examples: // Use environment variable passwordToSave = envPassword } else { - // Interactive prompt + // Interactive prompt - check if we're in a terminal + if !checkStdinIsTTY() { + return fmt.Errorf("TTY not detected - password required. Use --password flag or TIGER_NEW_PASSWORD environment variable") + } + fmt.Fprint(cmd.OutOrStdout(), "Enter password: ") - scanner := bufio.NewScanner(cmd.InOrStdin()) - if scanner.Scan() { - passwordToSave = scanner.Text() - } else { - if err := scanner.Err(); err != nil { - return fmt.Errorf("failed to read password: %w", err) - } - return fmt.Errorf("failed to read password: EOF") + bytePassword, err := readPasswordFromTerminal(int(os.Stdin.Fd())) + if err != nil { + return fmt.Errorf("failed to read password: %w", err) } + fmt.Fprintln(cmd.OutOrStdout()) // Print newline after hidden input + passwordToSave = string(bytePassword) if passwordToSave == "" { return fmt.Errorf("password cannot be empty") } @@ -319,19 +331,18 @@ Examples: // Save password using configured storage storage := password.GetPasswordStorage() - err = storage.Save(service, passwordToSave, dbSavePasswordRole) - if err != nil { + if err := storage.Save(service, passwordToSave, dbSavePasswordRole); err != nil { return fmt.Errorf("failed to save password: %w", err) } - fmt.Fprintf(cmd.OutOrStdout(), "Password saved successfully for service %s (role: %s)\n", + fmt.Fprintf(cmd.ErrOrStderr(), "Password saved successfully for service %s (role: %s)\n", *service.ServiceId, dbSavePasswordRole) return nil }, } // Add flags for db save-password command - cmd.Flags().StringVar(&dbSavePasswordValue, "password", "", "Password to save") + cmd.Flags().StringVarP(&dbSavePasswordValue, "password", "p", "", "Password to save") cmd.Flags().StringVar(&dbSavePasswordRole, "role", "tsdbadmin", "Database role/username") return cmd diff --git a/internal/tiger/cmd/db_test.go b/internal/tiger/cmd/db_test.go index 1d65849c..ffbe97c2 100644 --- a/internal/tiger/cmd/db_test.go +++ b/internal/tiger/cmd/db_test.go @@ -1193,24 +1193,27 @@ func TestDBSavePassword_InteractivePrompt(t *testing.T) { // Prepare the password input testPassword := "interactive-password-999" - inputReader := strings.NewReader(testPassword + "\n") - // Build the command with custom stdin - testRoot := buildRootCmd() - buf := new(bytes.Buffer) - testRoot.SetOut(buf) - testRoot.SetErr(buf) - testRoot.SetIn(inputReader) // Use our reader as stdin - testRoot.SetArgs([]string{"db", "save-password"}) // No --password flag and no env var + // Mock TTY check to return true (simulate terminal) + originalCheckStdinIsTTY := checkStdinIsTTY + checkStdinIsTTY = func() bool { + return true + } + defer func() { checkStdinIsTTY = originalCheckStdinIsTTY }() - // Execute the command - err = testRoot.Execute() + // Mock password reading to return our test password + originalReadPasswordFromTerminal := readPasswordFromTerminal + readPasswordFromTerminal = func(fd int) ([]byte, error) { + return []byte(testPassword), nil + } + defer func() { readPasswordFromTerminal = originalReadPasswordFromTerminal }() + + // Execute save-password without --password flag or env var + output, err := executeDBCommand("db", "save-password") if err != nil { t.Fatalf("Expected save-password to succeed with interactive input, got error: %v", err) } - output := buf.String() - // Verify the prompt was shown if !strings.Contains(output, "Enter password:") { t.Errorf("Expected password prompt, got: %s", output) @@ -1264,19 +1267,22 @@ func TestDBSavePassword_InteractivePromptEmpty(t *testing.T) { // Make sure TIGER_NEW_PASSWORD is not set os.Unsetenv("TIGER_NEW_PASSWORD") - // Prepare empty input (just a newline) - inputReader := strings.NewReader("\n") + // Mock TTY check to return true (simulate terminal) + originalCheckStdinIsTTY := checkStdinIsTTY + checkStdinIsTTY = func() bool { + return true + } + defer func() { checkStdinIsTTY = originalCheckStdinIsTTY }() - // Build the command with custom stdin - testRoot := buildRootCmd() - buf := new(bytes.Buffer) - testRoot.SetOut(buf) - testRoot.SetErr(buf) - testRoot.SetIn(inputReader) // Use our reader as stdin - testRoot.SetArgs([]string{"db", "save-password"}) // No --password flag or env var + // Mock password reading to return empty password + originalReadPasswordFromTerminal := readPasswordFromTerminal + readPasswordFromTerminal = func(fd int) ([]byte, error) { + return []byte(""), nil + } + defer func() { readPasswordFromTerminal = originalReadPasswordFromTerminal }() // Execute the command - err = testRoot.Execute() + _, err = executeDBCommand("db", "save-password") if err == nil { t.Fatal("Expected error when user provides empty password interactively") } diff --git a/specs/spec.md b/specs/spec.md index 934a6667..39d49041 100644 --- a/specs/spec.md +++ b/specs/spec.md @@ -31,7 +31,6 @@ mv tiger /usr/local/bin/ - `TIGER_CONFIG_DIR`: Configuration directory (default: ~/.config/tiger) - `TIGER_OUTPUT`: Default output format (json, yaml, table) - `TIGER_ANALYTICS`: Enable/disable usage analytics (true/false) -- `TIGER_NEW_PASSWORD`: Password to use for save-password command (used when --password flag not provided) ### Configuration File @@ -430,6 +429,12 @@ The `connect` and `psql` commands automatically handle authentication using: 2. `PGPASSWORD` environment variable 3. Interactive password prompt (if neither above is available) +**Password Input for save-password:** +The `save-password` command accepts passwords through three methods (in order of precedence): +1. `--password` flag with explicit value (highest precedence) +2. `TIGER_NEW_PASSWORD` environment variable +3. Interactive prompt (if neither provided and stdin is a TTY) + **Advanced psql Usage:** The `connect` and `psql` commands support passing additional flags directly to the psql client using the `--` separator. Any flags after `--` are passed through to psql unchanged, allowing full access to psql's functionality while maintaining tiger's connection and authentication handling.