auth: make logout credential deletion part of the success contract · Entire

auth: make logout credential deletion part of the success contract

91bb7e6→main·

toothbrush·1mo ago·5 files·+119 added/-58 removed

Review finding: RemoveCurrentContext/RemoveContext deleted the contexts.json entry first and swallowed keyring-delete failures, so a locked or failing keychain still printed "Logged out." while the long-lived refresh token survived, mintable by any keyring-capable process. Deletion now runs credentials-first (refresh slot before access, so a partial failure strands at worst the short-lived token) and any failure aborts with the entry intact — the benign direction: the context reads as not logged in and a retry no-ops the deletes.

Also rewords the login-token validation comments: opaque tokens pass the trust check but can no longer complete a login — RecordLoginContext is the sole persistence path and requires iss/handle claims; legacy opaque-token servers are intentionally unsupported.

Co-Authored-By: Claude Fable 5 noreply@anthropic.com

Sessions

ac4fd24010aaView transcript

Changes

5

// RemoveCurrentContext deletes the active context from contexts.json and
// its keyring token, clearing current_context. It is a no-op (returns nil)
// when there is no current context. Used by logout.
func RemoveCurrentContext() error {
    var svc, handle string
    if err := contexts.Modify(userdirs.Config(), func(f *contexts.File) (bool, error) {
        current := f.Find(f.CurrentContext)
        if current == nil {
            return false, nil
        }
        svc, handle = current.KeychainService, current.Handle
        // Logged out means logged out, no switch to another identity.
        f.Delete(current.Name)
        return true, nil
    }); err != nil {
        return fmt.Errorf("remove current context: %w", err)
    }
    return nil
}

// RemoveContext deletes the named context from contexts.json and its keyring
// tokens. A missing context is a no-op. Used by `logout --all-contexts` to
// drain every saved login.
func RemoveContext(name string) error {
    var svc, handle string
    f, err := contexts.Load(userdirs.Config())
    if err != nil {
        return fmt.Errorf("remove context %q: %w", name, err)
    }
    c := f.Find(name)
    if c == nil {
        return nil
    }
    if err := deleteContextKeychain(c.KeychainService, c.Handle); err != nil {
        return fmt.Errorf("remove credentials for %q: %w", name, err)
    }
    if err := contexts.Modify(userdirs.Config(), func(f *contexts.File) (bool, error) {
        c := f.Find(name)
        if c == nil {
            return false, nil
        }
        svc, handle = c.KeychainService, c.Handle
        f.Delete(name)
        return true, nil
    }); err != nil {
        return fmt.Errorf("remove context %q: %w", name, err)
    }
    deleteContextKeychain(svc, handle)
    return nil
}

// deleteContextKeychain removes a context's keyring slots.
func deleteContextKeychain(svc, handle string) error {
    if svc == "" || handle == "" {
        return nil
    }
    _ = tokenstore.Delete(svc, handle)                            //nolint:errcheck // best-effort
    _ = tokenstore.Delete(tokenstore.RefreshService(svc), handle) //nolint:errcheck // best-effort
    return nil
}

// validateReceivedToken checks the JWT token claims.
func validateReceivedToken(rawToken, issuerURL string, now time.Time) error {
    claims, err := tokens.ParseClaims(rawToken)
    if errors.Is(err, tokens.ErrUnsignedJWT) {
        return err //nolint:wrapcheck
    }
    if err != nil {
        return nil //nolint:nilerr
    }
    // iss check: the token must claim to come from the issuer we sent
    return nil
}

// TestRemoveContext_KeychainDeleteFailureAbortsLogout tests that the logout
// success contract is fulfilled when keyring deletion fails.
func TestRemoveContext_KeychainDeleteFailureAbortsLogout(t *testing.T) {
    ...
}