From f9bf4338b58dbf68c6be08ff72c3930c5eb33422 Mon Sep 17 00:00:00 2001 From: Aly Raffauf Date: Mon, 20 Jul 2026 14:12:57 -0400 Subject: [PATCH] auth: clear local credentials on logout despite server errors --- atproto/auth.go | 51 ++++++----- atproto/auth_test.go | 168 ++++++++++++++++++++++++++++++++++++ internal/cli/auth_logout.go | 1 + 3 files changed, 193 insertions(+), 27 deletions(-) diff --git a/atproto/auth.go b/atproto/auth.go index caf23bd..63b67bc 100644 --- a/atproto/auth.go +++ b/atproto/auth.go @@ -260,6 +260,10 @@ func (m *AuthManager) APIClient(ctx context.Context) (*atclient.APIClient, synta return client, passwordSession.AccountDID, nil } +// Logout removes the active account's credentials from the local keyring. +// Server-side revocation is best-effort; the local entry is removed even +// if the PDS rejects the revoke request. Returns ErrNotAuthenticated when +// there is no active account. func (m *AuthManager) Logout(ctx context.Context) error { account, err := m.activeAccount() if err != nil { @@ -269,37 +273,30 @@ func (m *AuthManager) Logout(ctx context.Context) error { if err != nil { return err } - if account.Method == AuthMethodOAuth { - err := m.app.Logout(ctx, did, "") - if err == nil { - return nil + switch account.Method { + case AuthMethodOAuth: + _ = m.app.Logout(ctx, did, "") + if err := m.store.DeleteSession(ctx, did, ""); err != nil { + return fmt.Errorf("clear local session: %w", err) } - // Corrupt or transient OAuth failure — force clear so the user can - // re-login instead of being locked out. - if deleteErr := m.store.DeleteSession(ctx, did, ""); deleteErr == nil { - return nil + case AuthMethodPassword: + passwordSession, err := m.store.GetPasswordSession(ctx, did) + if err != nil && !errors.Is(err, keyring.ErrNotFound) { + return err } - return err - } - - passwordSession, err := m.store.GetPasswordSession(ctx, did) - if err != nil { - if errors.Is(err, keyring.ErrNotFound) { - return ErrNotAuthenticated + if passwordSession != nil { + client := atclient.ResumePasswordSession(*passwordSession, nil) + if passwordAuth, ok := client.Auth.(*atclient.PasswordAuth); ok { + _ = passwordAuth.Logout(ctx, client.Client) + } } - return err - } - client := atclient.ResumePasswordSession(*passwordSession, nil) - passwordAuth, ok := client.Auth.(*atclient.PasswordAuth) - if !ok { - // Corrupt password session — force clear. - _ = m.store.DeletePasswordSession(ctx, did) - return nil - } - if err := passwordAuth.Logout(ctx, client.Client); err != nil { - return fmt.Errorf("revoke password session: %w", err) + if err := m.store.DeletePasswordSession(ctx, did); err != nil { + return fmt.Errorf("clear local session: %w", err) + } + default: + return fmt.Errorf("unsupported auth method %q", account.Method) } - return m.store.DeletePasswordSession(ctx, did) + return nil } func (m *AuthManager) LogoutAll(ctx context.Context) error { diff --git a/atproto/auth_test.go b/atproto/auth_test.go index 0703e77..5dff625 100644 --- a/atproto/auth_test.go +++ b/atproto/auth_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "testing" "github.com/bluesky-social/indigo/atproto/atclient" @@ -274,3 +275,170 @@ func TestLogoutAllRevokesEveryAccount(t *testing.T) { t.Fatalf("accounts after logout = %#v active %q", accounts, active) } } + +func TestPasswordLogoutClearsLocalOn401(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + w.Write([]byte(`{"error":"AuthenticationFailed","message":"Authentication failed"}`)) + })) + defer server.Close() + + store := testKeyringStore(newFakeKeyring()) + manager := newAuthManagerForTest("http://127.0.0.1:8095/callback", store) + ctx := context.Background() + did := mustDID(t, "did:plc:aaaabbbbccccddddeeeeffff") + + if err := store.SavePasswordSession(ctx, samplePasswordSession(did, server.URL)); err != nil { + t.Fatalf("SavePasswordSession: %v", err) + } + + if err := manager.Logout(ctx); err != nil { + t.Fatalf("Logout should clear local state despite 401, got: %v", err) + } + if _, err := store.GetPasswordSession(ctx, did); !errors.Is(err, keyring.ErrNotFound) { + t.Errorf("password session should have been cleared, got: %v", err) + } +} + +func TestPasswordLogoutClearsLocalOn500(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + store := testKeyringStore(newFakeKeyring()) + manager := newAuthManagerForTest("http://127.0.0.1:8095/callback", store) + ctx := context.Background() + did := mustDID(t, "did:plc:aaaabbbbccccddddeeeeffff") + + if err := store.SavePasswordSession(ctx, samplePasswordSession(did, server.URL)); err != nil { + t.Fatalf("SavePasswordSession: %v", err) + } + + if err := manager.Logout(ctx); err != nil { + t.Fatalf("Logout should clear local state despite 500, got: %v", err) + } + if _, err := store.GetPasswordSession(ctx, did); !errors.Is(err, keyring.ErrNotFound) { + t.Errorf("password session should have been cleared, got: %v", err) + } +} + +func TestPasswordLogoutFailsWhenLocalClearErrors(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + backend := newFakeKeyring() + store := testKeyringStore(backend) + manager := newAuthManagerForTest("http://127.0.0.1:8095/callback", store) + ctx := context.Background() + did := mustDID(t, "did:plc:aaaabbbbccccddddeeeeffff") + + if err := store.SavePasswordSession(ctx, samplePasswordSession(did, server.URL)); err != nil { + t.Fatalf("SavePasswordSession: %v", err) + } + + backend.deleteErr = errors.New("keyring daemon unavailable") + err := manager.Logout(ctx) + if err == nil { + t.Fatal("expected error when local clear fails, got nil") + } + if !strings.Contains(err.Error(), "clear local session") { + t.Errorf("error = %q, want substring %q", err, "clear local session") + } + if !strings.Contains(err.Error(), "keyring daemon unavailable") { + t.Errorf("error = %q, want substring %q", err, "keyring daemon unavailable") + } +} + +func TestLogoutAllClearsAllDespiteServerFailures(t *testing.T) { + var requests int + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if requests == 2 { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + w.Write([]byte(`{"error":"AuthenticationFailed","message":"Authentication failed"}`)) + return + } + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + store := testKeyringStore(newFakeKeyring()) + manager := newAuthManagerForTest("http://127.0.0.1:8095/callback", store) + ctx := context.Background() + first := mustDID(t, "did:plc:firstfirstfirstfirstfirst") + second := mustDID(t, "did:plc:secondsecondsecondsecond") + if err := store.SavePasswordSession(ctx, samplePasswordSession(first, server.URL)); err != nil { + t.Fatal(err) + } + if err := store.SavePasswordSession(ctx, samplePasswordSession(second, server.URL)); err != nil { + t.Fatal(err) + } + + if err := manager.LogoutAll(ctx); err != nil { + t.Fatalf("LogoutAll should clear all locals despite server failures, got: %v", err) + } + if _, err := store.GetPasswordSession(ctx, first); !errors.Is(err, keyring.ErrNotFound) { + t.Errorf("first session should have been cleared, got: %v", err) + } + if _, err := store.GetPasswordSession(ctx, second); !errors.Is(err, keyring.ErrNotFound) { + t.Errorf("second session should have been cleared, got: %v", err) + } +} + +func TestOAuthLogoutClearsLocalOnServerError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + store := testKeyringStore(newFakeKeyring()) + manager := newAuthManagerForTest("http://127.0.0.1:8095/callback", store) + ctx := context.Background() + did := mustDID(t, "did:plc:aaaabbbbccccddddeeeeffff") + + session := sampleSession(did) + session.DPoPPrivateKeyMultibase = mustGenerateDpopKey(t) + session.HostURL = server.URL + if err := store.SaveSession(ctx, session); err != nil { + t.Fatalf("SaveSession: %v", err) + } + + if err := manager.Logout(ctx); err != nil { + t.Fatalf("Logout should clear local state despite server error, got: %v", err) + } + if _, err := store.GetSession(ctx, did, ""); !errors.Is(err, keyring.ErrNotFound) { + t.Errorf("OAuth session should have been cleared, got: %v", err) + } +} + +// TestPasswordLogoutClearsAccountWithMissingSession verifies that Logout +// removes the account from the index even when the session secret is already +// gone, so the user is never locked out by a stale index entry. +func TestPasswordLogoutClearsAccountWithMissingSession(t *testing.T) { + backend := newFakeKeyring() + store := testKeyringStore(backend) + manager := newAuthManagerForTest("http://127.0.0.1:8095/callback", store) + ctx := context.Background() + did := mustDID(t, "did:plc:aaaabbbbccccddddeeeeffff") + + if err := store.SavePasswordSession(ctx, samplePasswordSession(did, "https://pds.example")); err != nil { + t.Fatalf("SavePasswordSession: %v", err) + } + delete(backend.secrets, backendKey(keyringService, passwordKey(did.String()))) + + if err := manager.Logout(ctx); err != nil { + t.Fatalf("Logout should clear account despite missing session, got: %v", err) + } + accounts, _, err := store.Accounts() + if err != nil { + t.Fatal(err) + } + if len(accounts) != 0 { + t.Errorf("account should have been removed from index, got: %v", accounts) + } +} diff --git a/internal/cli/auth_logout.go b/internal/cli/auth_logout.go index 4cc8525..1a3db80 100644 --- a/internal/cli/auth_logout.go +++ b/internal/cli/auth_logout.go @@ -13,6 +13,7 @@ var authLogoutAll bool var authLogoutCmd = &cobra.Command{ Use: "logout", Short: "Log out of your AT Protocol account", + Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { var err error if authLogoutAll { -- 2.51.2