PLAT-1314/fix: preserve keychain lookup and deletion errors

README.md

@@ -122,6 +122,13 @@ API key resolution order (highest precedence first):
 
 Keys are obtained from the [Two merchant portal](https://portal.two.inc).
 
+If a keychain lookup fails, twoctl reports the underlying error rather than
+claiming no key is stored. On headless Linux without a running Secret
+Service/DBus session, the error explains the required backend and suggests
+`TWO_API_KEY` for headless use. Explicit `--api-key` and `TWO_API_KEY` values
+bypass the keychain. A missing key still prompts you to run `twoctl auth login`;
+keychain failures never fall back to storing keys in a file.
+
 ### Configuration directory
 
 Set `TWOCTL_CONFIG_HOME` to use a dedicated twoctl configuration directory:

internal/config/config.go

@@ -15,6 +15,7 @@ package config
 import (
 	"errors"
 	"fmt"
+	"net"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -126,7 +127,11 @@ func resolveKey(flagKey, ctxName string) (string, string, error) {
 		return v, "$" + envVar, nil
 	}
 	if ctxName != "" {
-		if k, err := keyring.Get(keyringService, keyringAccount(ctxName)); err == nil && k != "" {
+		k, err := keyring.Get(keyringService, keyringAccount(ctxName))
+		if err != nil && !errors.Is(err, keyring.ErrNotFound) {
+			return "", "", fmt.Errorf("reading keychain for context %q: %w", ctxName, wrapKeyringErr(err))
+		}
+		if err == nil && k != "" {
 			return k, fmt.Sprintf("keychain (context %s)", ctxName), nil
 		}
 	}
@@ -396,21 +401,29 @@ func wrapKeyringErr(err error) error {
 	msg := err.Error()
 	if strings.Contains(msg, "secret-service") ||
 		strings.Contains(msg, "org.freedesktop.secrets") ||
-		strings.Contains(msg, "DBUS_SESSION_BUS_ADDRESS") {
+		strings.Contains(msg, "DBUS_SESSION_BUS_ADDRESS") ||
+		strings.Contains(msg, "dbus-launch") || isKeyringDialError(err) {
 		return fmt.Errorf("no OS keychain backend available - twoctl needs libsecret + a running secret-service (gnome-keyring or KeePassXC). "+
 			"On headless servers, set TWO_API_KEY in the environment instead. (cause: %w)", err)
 	}
 	return err
 }
 
+// isKeyringDialError recognizes the socket errors returned directly by godbus.
+// Inspect the error chain so wrapped dial failures retain the same guidance.
+func isKeyringDialError(err error) bool {
+	var opErr *net.OpError
+	return errors.As(err, &opErr) && opErr.Op == "dial"
+}
+
 // DeleteKey removes the keychain entry for the given context. A missing
 // entry is not an error.
 func DeleteKey(ctxName string) error {
 	err := keyring.Delete(keyringService, keyringAccount(ctxName))
 	if errors.Is(err, keyring.ErrNotFound) {
 		return nil
 	}
-	return err
+	return wrapKeyringErr(err)
 }
 
 // HasStoredKey reports whether the keychain currently holds a key for the

internal/config/keyring_errors_test.go

@@ -0,0 +1,129 @@
+package config
+
+import (
+	"errors"
+	"fmt"
+	"net"
+	"os/exec"
+	"strings"
+	"testing"
+
+	"github.com/zalando/go-keyring"
+)
+
+func TestResolvePreservesKeychainFailure(t *testing.T) {
+	for _, message := range []string{
+		"could not connect to secret-service",
+		"org.freedesktop.secrets returned an error",
+		"DBUS_SESSION_BUS_ADDRESS not set",
+		"User interaction is not allowed",
+	} {
+		t.Run(message, func(t *testing.T) {
+			withTempHome(t)
+			t.Setenv("TWO_API_KEY", "")
+			failure := errors.New(message)
+			keyring.MockInitWithError(failure)
+			t.Cleanup(keyring.MockInit)
+			_, err := Resolve("", "sandbox", "")
+			if !errors.Is(err, failure) {
+				t.Fatalf("lost underlying keychain failure: %v", err)
+			}
+			if !strings.Contains(err.Error(), "sandbox") {
+				t.Fatalf("error omitted the affected context: %v", err)
+			}
+		})
+	}
+}
+
+func TestKeychainOperationsExplainMissingBackend(t *testing.T) {
+	for name, operation := range map[string]func() error{
+		"read":   func() error { _, _, err := resolveKey("", "sandbox"); return err },
+		"write":  func() error { return StoreKey("sandbox", "test-key") },
+		"delete": func() error { return DeleteKey("sandbox") },
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Setenv("TWO_API_KEY", "")
+			keyring.MockInitWithError(errors.New("DBUS_SESSION_BUS_ADDRESS not set"))
+			t.Cleanup(keyring.MockInit)
+			err := operation()
+			if err == nil || !strings.Contains(err.Error(), "libsecret") || !strings.Contains(err.Error(), "TWO_API_KEY") {
+				t.Fatalf("expected backend guidance, got %v", err)
+			}
+		})
+	}
+}
+
+func TestExplicitKeysBypassUnavailableKeychain(t *testing.T) {
+	for _, useFlag := range []bool{false, true} {
+		withTempHome(t)
+		t.Setenv("TWO_API_KEY", "env-key")
+		keyring.MockInitWithError(errors.New("DBUS_SESSION_BUS_ADDRESS not set"))
+		t.Cleanup(keyring.MockInit)
+		flag, want, source := "", "env-key", "$TWO_API_KEY"
+		if useFlag {
+			flag, want, source = "flag-key", "flag-key", "--api-key flag"
+		}
+		resolved, err := Resolve(flag, "sandbox", "")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if resolved.APIKey != want || resolved.Source != source {
+			t.Fatalf("wrong explicit key/source: %+v", resolved)
+		}
+	}
+}
+
+func TestMissingKeyStillReportsNoAPIKey(t *testing.T) {
+	withTempHome(t)
+	t.Setenv("TWO_API_KEY", "")
+	keyring.MockInitWithError(keyring.ErrNotFound)
+	t.Cleanup(keyring.MockInit)
+	_, err := Resolve("", "sandbox", "")
+	if !errors.Is(err, ErrNoAPIKey) {
+		t.Fatalf("missing key should retain login guidance: %v", err)
+	}
+	if err := DeleteKey("sandbox"); err != nil {
+		t.Fatalf("deleting an absent key should remain a no-op: %v", err)
+	}
+}
+
+func TestKeychainOperationsExplainDBusConnectionFailures(t *testing.T) {
+	failures := []error{
+		&exec.Error{Name: "dbus-launch", Err: exec.ErrNotFound},
+		&net.OpError{Op: "dial", Net: "unix", Err: errors.New("connect: no such file or directory")},
+		&net.OpError{Op: "dial", Net: "tcp", Err: errors.New("connect: connection refused")},
+	}
+	for _, failure := range failures {
+		t.Run(failure.Error(), func(t *testing.T) {
+			t.Setenv("TWO_API_KEY", "")
+			keyring.MockInitWithError(fmt.Errorf("backend: %w", failure))
+			t.Cleanup(keyring.MockInit)
+			for name, operation := range map[string]func() error{
+				"read":   func() error { _, _, err := resolveKey("", "sandbox"); return err },
+				"write":  func() error { return StoreKey("sandbox", "test-key") },
+				"delete": func() error { return DeleteKey("sandbox") },
+			} {
+				t.Run(name, func(t *testing.T) {
+					err := operation()
+					if !errors.Is(err, failure) {
+						t.Fatalf("lost backend cause: %v", err)
+					}
+					if !strings.Contains(err.Error(), "libsecret") || !strings.Contains(err.Error(), "TWO_API_KEY") {
+						t.Fatalf("missing backend guidance: %v", err)
+					}
+				})
+			}
+		})
+	}
+}
+
+func TestUnrelatedKeyringErrorsRemainUnchanged(t *testing.T) {
+	for _, failure := range []error{
+		errors.New("User interaction is not allowed"),
+		&net.OpError{Op: "read", Net: "unix", Err: errors.New("permission denied")},
+	} {
+		if got := wrapKeyringErr(failure); got != failure {
+			t.Fatalf("unrelated error was reclassified: %v", got)
+		}
+	}
+}