fix: detect a selected-but-unusable OS keyring and fall back gracefully#10168
Open
kundansable wants to merge 1 commit into
Open
fix: detect a selected-but-unusable OS keyring and fall back gracefully#10168kundansable wants to merge 1 commit into
kundansable wants to merge 1 commit into
Conversation
evaluate_and_patch_config() only disabled USE_OS_SECRET_STORAGE when keyring.get_keyring().name == 'fail Keyring'. On Debian 13 over RDP, a real backend (e.g. SecretService) is selected because no D-Bus/GNOME Keyring session is running, so the name check passes even though every actual keyring call fails — leaving the crypt key unset and every operation raising a bare CryptKeyMissing. Probe the selected backend with a harmless read of a never-existing entry after selection. A healthy backend returns None without prompting; an unusable one raises, and we disable USE_OS_SECRET_STORAGE so the app falls back to the master-password / in-app crypt key mechanism instead of failing outright. Fixes pgadmin-org#10107
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe configuration now probes non-failing keyring backends before enabling OS secret storage. Probe failures disable OS secret storage and clear ChangesOS secret storage configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #10107 — in Desktop mode on Debian 13 (Trixie) under an RDP session (no D-Bus / GNOME Keyring available), pgAdmin fails with a bare
CryptKeyMissingon every server-registration or connect attempt, instead of falling back to the master-password mechanism.Root Cause
evaluate_and_patch_config()only disablesUSE_OS_SECRET_STORAGEwhenkeyring.get_keyring().name == 'fail Keyring'— i.e. when thekeyringlibrary couldn't find any backend at all. On Debian 13 over RDP, a real backend (e.g.SecretService) is selected (so the name check passes), but it's non-functional because there's no D-Bus/GNOME Keyring session running in that environment. Every actual keyring read/write then fails silently at a different layer, the crypt key never gets set, andConnection.connect()unconditionally raisesCryptKeyMissingwhenever the key is absent — with no recovery path.Fix
After a keyring backend is selected, probe it with a harmless read of a never-existing entry (
keyring.get_password('pgAdmin4', 'entry_to_check_keyring_access')). A healthy backend returnsNonewithout prompting (OS keyrings only prompt for existing entries). An unusable backend raises an exception on this probe — in that case, disableUSE_OS_SECRET_STORAGEso the app falls back to the master-password / in-app crypt key mechanism instead of failing outright.Test Steps
(Requires a Debian 13 desktop-mode environment without a working D-Bus/keyring session — e.g. an RDP session, or simulate by disabling/blocking the keyring daemon.)
pgadmin.utils.exception.CryptKeyMissing.CryptKeyMissingerror.USE_OS_SECRET_STORAGEstays enabled and passwords are still stored via the OS keyring as before (no unnecessary fallback).Summary by CodeRabbit