Fix Query Tool password re-prompt loop for unsaved passwords (#9091)#10073
Open
dpage wants to merge 2 commits into
Open
Fix Query Tool password re-prompt loop for unsaved passwords (#9091)#10073dpage wants to merge 2 commits into
dpage wants to merge 2 commits into
Conversation
…n-org#9091) When a tool (Query Tool, View/Edit Data, etc.) opens a connection for a server whose password was not saved, it relies on the password cached on the server manager. If that cached password is unavailable, the tool prompts for it. The entered password was POSTed to the connect_server endpoint, which short-circuited with "Server connected" whenever the server's primary connection was already established -- silently discarding the entered password. The tool's own connection therefore still had no password and re-prompted immediately, producing an infinite prompt loop in which the re-entered password appeared to be rejected. Cache the entered password on the server manager (encrypted) in that short-circuit path so the tool's connection can reuse it. The password overwrites any cached value, so a regenerated short-lived cloud auth token (AWS RDS IAM / Azure Entra) takes effect immediately.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis pull request fixes a regression where long passwords (such as AWS RDS authentication tokens) were not cached between initial server connection and subsequent SQL Editor queries, causing re-prompts and authentication failures. The fix extracts the password from client requests and stores it on the server manager when the primary connection is already established. ChangesPassword Caching for Query Tool
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/pgadmin/tools/sqleditor/utils/tests/test_cache_manager_password.py (1)
37-64: ⚡ Quick winConsider adding a JSON data scenario for complete coverage.
The stack context indicates that
_cache_manager_password_from_requestshould extract passwords from "request form or JSON," but all current scenarios only test the form data path. Consider adding a scenario that supplies the password viarequest.data(JSON) instead ofrequest.formto ensure both extraction paths are covered.📝 Suggested JSON scenario
+ ('When a password is supplied via JSON it is encrypted and cached', dict( + form_data={}, + json_data={'password': LONG_TOKEN}, + crypt_key_present=True, + expect_cached=True, + )),Then update line 77 to:
- mock_request.data = None + mock_request.data = getattr(self, 'json_data', {})
Note: The RUF012 warning about mutable class attributes is a false positive. The
scenariospattern is an established convention in pgAdmin'sBaseTestGenerator-based tests, where each test run receives independent scenario instances.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/tools/sqleditor/utils/tests/test_cache_manager_password.py` around lines 37 - 64, Add a test scenario that covers JSON payload extraction by supplying the password in request.data instead of request.form: update the scenarios list (alongside the existing form-based cases) to include a case where form_data={} (or omitted) and json_data={'password': LONG_TOKEN}, with crypt_key_present=True and expect_cached=True so _cache_manager_password_from_request is exercised for the JSON path; ensure any existing_password/expect_cached variants you need are mirrored if you want overwrite behavior tested.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 2708-2739: The function _cache_manager_password_from_request
currently calls json.loads(request.data) outside the try/except, which can raise
JSONDecodeError and crash the endpoint; move the JSON parsing into the existing
try/except (or add a small try/except around json.loads) so that invalid JSON is
treated as a no-op and the function returns silently; ensure you catch
json.JSONDecodeError (or Exception) and return early, keeping
manager._update_password(encrypt(...)) and manager.update_session inside the try
block so they only run with valid parsed data.
---
Nitpick comments:
In `@web/pgadmin/tools/sqleditor/utils/tests/test_cache_manager_password.py`:
- Around line 37-64: Add a test scenario that covers JSON payload extraction by
supplying the password in request.data instead of request.form: update the
scenarios list (alongside the existing form-based cases) to include a case where
form_data={} (or omitted) and json_data={'password': LONG_TOKEN}, with
crypt_key_present=True and expect_cached=True so
_cache_manager_password_from_request is exercised for the JSON path; ensure any
existing_password/expect_cached variants you need are mirrored if you want
overwrite behavior tested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18d32ae9-cbc2-4d7f-bc6e-5304c382f603
📒 Files selected for processing (3)
docs/en_US/release_notes_9_16.rstweb/pgadmin/tools/sqleditor/__init__.pyweb/pgadmin/tools/sqleditor/utils/tests/test_cache_manager_password.py
Move request-body JSON parsing inside the try/except so malformed request data is treated as a silent no-op (logged and swallowed) rather than propagating a JSONDecodeError and turning the connect_server endpoint's 'Server connected' response into a 500. Add a regression test scenario for the malformed-JSON case. Addresses CodeRabbit review feedback on pgadmin-org#10073.
There was a problem hiding this comment.
Pull request overview
Fixes an infinite password re-prompt loop affecting Query Tool (and other tool-specific connections) when a server password is not saved, by caching the user-entered password on the ServerManager even when the primary connection is already established.
Changes:
- Cache the password from the
connect_serverrequest on theServerManager(encrypted) in the “already connected” short-circuit path. - Add unit tests covering caching, overwriting, and no-op scenarios (including malformed JSON handling).
- Document the fix in the v9.16 release notes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web/pgadmin/tools/sqleditor/init.py | Add helper to cache request password (encrypted) when connect_server short-circuits as already connected. |
| web/pgadmin/tools/sqleditor/utils/tests/test_cache_manager_password.py | Add regression tests for password caching behavior across multiple scenarios. |
| docs/en_US/release_notes_9_16.rst | Add release note entry for Issue #9091. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2741
to
+2742
| except Exception as e: | ||
| current_app.logger.exception(e) |
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 an infinite password-prompt loop in the Query Tool (and other tools that open their own connections) for servers whose password was not saved — most visibly reported with long, short-lived cloud auth tokens used as the password (AWS RDS IAM, Azure Entra).
Root cause
When a tool such as the Query Tool opens a connection for a server whose password is not saved, it relies on the password cached on the
ServerManager. If that cached password is unavailable, the tool returns428and the UI prompts for the password. The entered password is POSTed to thesqleditor.connect_serverendpoint.connect_servershort-circuits with"Server connected"whenever the server's primary connection is already established — and in doing so silently discards the password the user just entered. The tool's own connection therefore still has no password and re-prompts immediately, producing an infinite loop in which the re-entered password appears to be rejected.Fix
In that short-circuit path, cache the entered password on the
ServerManager(encrypted) so the tool's connection can reuse it. The password overwrites any previously cached value, so a regenerated short-lived token takes effect immediately rather than the loop retrying a stale/expired one.The change is confined to the
connect_serverendpoint via a small helper; it is a no-op when no password is supplied or the encryption key is unavailable.Test plan
test_cache_manager_password.py(5 scenarios): password supplied is encrypted and cached; an existing cached password is overwritten; no-ops when the password is absent, empty, or the crypt key is missing. The cached value is verified to decrypt back to the original token intact.test_new_connection_dialogstill passes.pycodestyleclean.Note for reviewers
Several reporters (and maintainers) could not reproduce the original symptom in every environment, and a length/content correlation in the thread remains unexplained. This change definitively breaks the prompt loop and makes a correctly re-entered password take effect, matching the primary reported symptom. I've asked the reporters on the issue for their server mode / worker count / workspace-restore details in case a complementary fix (more reliable
manager.passwordpersistence) is also warranted.Closes #9091
Summary by CodeRabbit
Release Notes