Read password from PassFile when connecting.#9810
Read password from PassFile when connecting.#9810m000 wants to merge 4 commits intopgadmin-org:masterfrom
Conversation
|
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)
WalkthroughReordered credential fallback in psycopg3 connection: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/utils/driver/psycopg3/connection.py`:
- Around line 317-320: The current logic creates a Path for kwargs['passfile']
and calls passfile.read_text() without validating that the value is non-empty
and points to a regular readable file; update the code around the passfile
handling so that if kwargs['passfile'] is an empty string it is treated as None,
then verify passfile.exists() and passfile.is_file() before calling
passfile.read_text(), and wrap the read_text() call in a try/except that catches
IsADirectoryError, FileNotFoundError, PermissionError, and UnicodeDecodeError
(and convert or re-raise them as a clear connection error or log appropriately)
so that reading failures do not propagate uncaught from the code that sets the
password variable.
🪄 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: 340f95c2-cfcc-42e7-a15f-f262c72b2620
📒 Files selected for processing (1)
web/pgadmin/utils/driver/psycopg3/connection.py
m000
force-pushed
the
passfile-fix
branch
2 times, most recently
from
55cf4b6 to
50a03d5
Compare
Use PassFile to retrieve a password when one is not retrieved by other means. The PassFile setting was already passed to connect() but was never read.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/utils/driver/psycopg3/connection.py`:
- Around line 316-317: The code constructs Path(kwargs['passfile']) whenever the
'passfile' key exists, which raises TypeError if its value is None; change the
guard so you only call Path(...) when the retrieved value is not None/empty
(e.g., use val = kwargs.get('passfile') and then passfile = Path(val) only if
val is truthy), ensuring the variable passfile remains None otherwise so
downstream exception handling around connect() or the passfile logic can run
safely.
🪄 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: a364471d-0bf2-4720-9580-b8a79cbfc2c3
📒 Files selected for processing (1)
web/pgadmin/utils/driver/psycopg3/connection.py
| passfile = Path(kwargs['passfile']) if 'passfile' in kwargs else None | ||
| if not password and passfile: | ||
| try: | ||
| password = passfile.read_text(encoding='utf-8').strip() |
There was a problem hiding this comment.
passfile is read by psycopg driver. We just need to pass the args in connection.
There was a problem hiding this comment.
You're right! It turns out that passfile is indeed passed to psycopg. What was confusing is that psycopg won't emit any warnings if the format is invalid. In my case it was a docker secret containing only the password, which of course failed (silently).
So I guess that the initially proposed changes are not needed.
But the logic built around passfile in connect() was still funny: A potential passfile kwarg is used for a check, but then the connection is created by whatever passfile is picked up internally by ServerManager, ignoring the kwarg.
I tried to clean up the logic, so that the value used by ServerManager is also used for the checks in connect(), and warn if a kwarg is supplied and is different than that. (This probably won't happen with the current code AFAICT, but updates may change that.)
Let me know what you think and, if the changes look ok, whether I should update the description of this PR or close it and create a new one.
This reverts commit 7c9be2d.
Checking for a passfile is done by using the value returned by the ServerManager, which will also be used when constructing the connection string. An extra check is added, to warn if passfile is also provided as a kwarg to Connection::connect() and differs from the passfile picked up by ServerManager.
2 participants
Use PassFile to retrieve a password when one is not retrieved by other means. The PassFile setting was already passed to connect() but was never read.
Summary by CodeRabbit