diff --git a/web/pgadmin/browser/server_groups/servers/__init__.py b/web/pgadmin/browser/server_groups/servers/__init__.py index cfcb324c4d8..be61770778c 100644 --- a/web/pgadmin/browser/server_groups/servers/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/__init__.py @@ -44,7 +44,7 @@ from sqlalchemy.orm.attributes import flag_modified from pgadmin.utils.preferences import Preferences from .... import socketio as sio -from pgadmin.utils import get_complete_file_path +from pgadmin.utils import get_complete_file_path, str_to_bool from pgadmin.settings.utils import with_object_filters from pgadmin.utils.server_access import get_server, \ get_user_server_query, get_server_group @@ -1618,6 +1618,12 @@ def connect(self, gid, sid, is_qt=False, server=None): passfile = None tunnel_password = None save_password = False + # Distinguishes "the caller explicitly said false" from "the + # caller didn't mention save_password at all" -- only the former + # should clear an existing saved credential (see the success + # branch below); legacy callers that omit the field must not have + # a saved password silently wiped out from under them. + save_password_provided = False save_tunnel_password = False prompt_password = False prompt_tunnel_password = False @@ -1685,8 +1691,15 @@ def connect(self, gid, sid, is_qt=False, server=None): password = conn_passwd or server.password else: password = data['password'] if 'password' in data else None - save_password = data['save_password']\ - if 'save_password' in data else False + # The password-prompt dialog seeds its checkbox from the + # server's current save_password setting (see + # get_response_for_password) and always sends its state, so + # this reflects the user's explicit choice -- including + # unchecking it for a server previously configured to save + # its password. + save_password_provided = 'save_password' in data + save_password = str_to_bool( + data['save_password'] if save_password_provided else False) try: # Encrypt the password before saving with user's login @@ -1737,6 +1750,12 @@ def connect(self, gid, sid, is_qt=False, server=None): # 1 is True in SQLite as no boolean type if _is_non_owner(server): setattr(shared_server, 'save_password', 1) + # `server` is a detached overlay (see + # get_shared_server_properties) built before this + # write, so it won't pick up the SharedServer + # change on its own -- keep it in sync since the + # connect response below reports its state. + server.save_password = 1 else: setattr(server, 'save_password', 1) @@ -1754,6 +1773,28 @@ def connect(self, gid, sid, is_qt=False, server=None): manager.release(database=server.maintenance_db) conn = None + return internal_server_error(errormsg=str(e)) + elif save_password_provided and not save_password and \ + server.save_password and config.ALLOW_SAVE_PASSWORD: + # The user explicitly unticked "Save Password" on a server + # that had one saved -- clear it instead of leaving the + # now-stale credential and flag in place. + try: + if _is_non_owner(server): + setattr(shared_server, 'save_password', 0) + setattr(shared_server, 'password', None) + # Keep the detached overlay in sync -- see the + # comment in the save_password branch above. + server.save_password = 0 + else: + setattr(server, 'save_password', 0) + setattr(server, 'password', None) + db.session.commit() + except Exception as e: + current_app.logger.exception(e) + manager.release(database=server.maintenance_db) + conn = None + return internal_server_error(errormsg=str(e)) if save_tunnel_password and config.ALLOW_SAVE_TUNNEL_PASSWORD: @@ -2195,6 +2236,7 @@ def get_response_for_password(self, server, status, prompt_password=False, "service": server.service, "prompt_tunnel_password": prompt_tunnel_password, "prompt_password": prompt_password, + "save_password": bool(server.save_password), "allow_save_password": True if config.ALLOW_SAVE_PASSWORD and 'allow_save_password' in session and @@ -2217,6 +2259,7 @@ def get_response_for_password(self, server, status, prompt_password=False, "errmsg": errmsg, "service": server.service, "prompt_password": True, + "save_password": bool(server.save_password), "allow_save_password": True if config.ALLOW_SAVE_PASSWORD and 'allow_save_password' in session and diff --git a/web/pgadmin/static/js/Dialogs/ConnectServerContent.jsx b/web/pgadmin/static/js/Dialogs/ConnectServerContent.jsx index 796e1441c3a..0f07430b0b7 100644 --- a/web/pgadmin/static/js/Dialogs/ConnectServerContent.jsx +++ b/web/pgadmin/static/js/Dialogs/ConnectServerContent.jsx @@ -26,7 +26,10 @@ export default function ConnectServerContent({closeModal, data, onOK, setHeight, tunnel_password: '', save_tunnel_password: false, password: '', - save_password: false, + // Seed the checkbox from the server's current setting so that, for a + // server already configured to save its password, the checkbox + // reflects that instead of always defaulting to unchecked. + save_password: Boolean(data?.save_password), }); const onTextChange = (e, id) => { @@ -119,8 +122,10 @@ export default function ConnectServerContent({closeModal, data, onOK, setHeight, } if(data.prompt_password) { postFormData.append('password', formData.password); - formData.save_password && - postFormData.append('save_password', formData.save_password); + // Always send the checkbox state (rather than only when + // checked) so the backend can tell "explicitly unchecked" + // apart from "field not sent". + postFormData.append('save_password', formData.save_password); } onOK?.(postFormData); closeModal(); diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index 8080d220a54..2511d8c7db4 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -42,7 +42,7 @@ from pgadmin.tools.sqleditor.utils.update_session_grid_transaction import \ update_session_grid_transaction from pgadmin.utils import PgAdminModule -from pgadmin.utils import get_storage_directory +from pgadmin.utils import get_storage_directory, str_to_bool from pgadmin.utils.ajax import make_json_response, bad_request, \ success_return, internal_server_error, service_unavailable, gone from pgadmin.utils.driver import get_driver @@ -267,6 +267,7 @@ def initialize_viewdata(trans_id, cmd_type, obj_type, sgid, sid, did, obj_id): "username": user or server.username, "errmsg": msg, "prompt_password": True, + "save_password": bool(server.save_password), "allow_save_password": True if ALLOW_SAVE_PASSWORD and session.get('allow_save_password', None) @@ -592,6 +593,7 @@ def _init_sqleditor(trans_id, connect, sgid, sid, did, dbname=None, **kwargs): "username": user or server.username, "errmsg": msg, "prompt_password": True, + "save_password": bool(server.save_password), "allow_save_password": True if ALLOW_SAVE_PASSWORD and session.get('allow_save_password', None) @@ -2713,7 +2715,7 @@ def connect_server(sid): # password the user just entered at that prompt is cached here so the # tool's connection can use it, instead of being discarded and # re-prompted in a loop. - _cache_manager_password_from_request(manager) + _cache_manager_password_from_request(manager, server) return make_json_response( success=1, info=gettext("Server connected."), @@ -2726,7 +2728,7 @@ def connect_server(sid): ) -def _cache_manager_password_from_request(manager): +def _cache_manager_password_from_request(manager, server=None): """ Cache the password supplied with the current request (from a tool's password prompt) onto the server manager, so that connections opened by @@ -2737,6 +2739,13 @@ def _cache_manager_password_from_request(manager): password, so a freshly entered credential (e.g. a regenerated, short-lived cloud auth token) takes effect immediately. + When "Save Password" is requested and allowed, the freshly entered + password is also persisted to the server record (overwriting any stale + stored ciphertext). Without this, a rotated/regenerated password entered + at the prompt would work for the current session only and the tool would + keep re-using the stale saved password and re-prompt on the next + connection. + This is best-effort: any failure (including malformed request data) is logged and swallowed so it never turns the caller's "Server connected" response into a 500 error. @@ -2757,12 +2766,116 @@ def _cache_manager_password_from_request(manager): if not crypt_key_present: return - manager._update_password(encrypt(password, crypt_key)) + # This request never actually uses `password` to open a connection + # (the manager's primary connection was already established + # beforehand), so it must be validated against the server before + # caching it on the manager or persisting it -- otherwise a typo at + # the prompt would silently replace a working password, for the + # current session as well as in durable storage. + if not _password_is_valid(manager, password): + return + + enc_password = encrypt(password, crypt_key) + manager._update_password(enc_password) manager.update_session() + + if server is None or not ALLOW_SAVE_PASSWORD: + return + + save_password_provided = 'save_password' in data + save_password = str_to_bool(data.get('save_password', False)) + + # Persist the freshly entered password if the user asked to save + # it, so the stale stored ciphertext is replaced. An explicit + # false instead clears any previously saved credential -- mirrors + # the same "Save Password" opt-out handling in + # browser.server_groups.servers.ServerNode.connect -- so + # unchecking the box here doesn't leave a stale saved password. + if save_password: + _persist_saved_password(server, enc_password) + elif save_password_provided: + _clear_saved_password(server) except Exception as e: current_app.logger.exception(e) +def _password_is_valid(manager, password): + """ + Verify that `password` (plaintext) actually authenticates against the + server, using a standalone connection that is closed immediately + afterwards -- it is never registered with the manager. + """ + import psycopg + try: + conn_string = manager.create_connection_string( + manager.db, manager.user, password) + test_conn = psycopg.Connection.connect( + conn_string, connect_timeout=10) + test_conn.close() + return True + except psycopg.Error as e: + current_app.logger.info( + 'Not persisting the re-entered password: it failed ' + f'validation against the server.\nError: {e}' + ) + return False + + +def _get_save_password_target(server): + """ + Return the record ("save_password"/"password" live on the owned Server + row, or on the current user's SharedServer row for a shared server they + don't own). + """ + from pgadmin.browser.server_groups.servers import ( + ServerModule, _is_non_owner) + + if _is_non_owner(server): + shared_server = ServerModule.get_shared_server( + server, server.servergroup_id) + if shared_server is not None: + return shared_server + return server + + +def _persist_saved_password(server, enc_password): + """ + Persist the encrypted password to the server record (owned or shared), + replacing any stale stored ciphertext. + """ + from pgadmin.model import db + + target = _get_save_password_target(server) + setattr(target, 'save_password', 1) + setattr(target, 'password', enc_password) + try: + db.session.commit() + except Exception: + db.session.rollback() + raise + + +def _clear_saved_password(server): + """ + Clear a previously saved password on the owned or shared server record, + so an explicit "Save Password" opt-out doesn't leave a stale saved + credential behind. + """ + from pgadmin.model import db + + target = _get_save_password_target(server) + if not target.save_password: + return + + setattr(target, 'save_password', 0) + setattr(target, 'password', None) + try: + db.session.commit() + except Exception: + db.session.rollback() + raise + + @blueprint.route( '/filter_dialog/', methods=["PUT"], endpoint='set_filter_data' diff --git a/web/pgadmin/tools/sqleditor/tests/test_persist_saved_password.py b/web/pgadmin/tools/sqleditor/tests/test_persist_saved_password.py new file mode 100644 index 00000000000..875ee684902 --- /dev/null +++ b/web/pgadmin/tools/sqleditor/tests/test_persist_saved_password.py @@ -0,0 +1,122 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +""" +Unit tests for the reconnect-path password persistence added for #10128. + +These exercise _persist_saved_password's owner/shared routing and +_password_is_valid's pass/fail behaviour directly, with the DB and psycopg +layers mocked out -- they don't need a live Postgres server connection. +""" + +import unittest +from unittest.mock import patch, MagicMock + +from pgadmin.utils.route import BaseTestGenerator +from pgadmin.utils import str_to_bool +from pgadmin.tools.sqleditor import _persist_saved_password, \ + _password_is_valid + + +class _NoServerSetupMixin: + """Skip BaseTestGenerator.setUp's Postgres connection -- these tests + exercise pure Python logic with mocked collaborators.""" + + def setUp(self): + unittest.TestCase.setUp(self) + + +class TestPersistSavedPasswordOwner(_NoServerSetupMixin, BaseTestGenerator): + """An owned server's rotated password is written to the Server row + itself.""" + + def runTest(self): + server = MagicMock(shared=False, user_id=1) + servers_mod = 'pgadmin.browser.server_groups.servers.ServerModule' + + with patch('pgadmin.model.db') as mock_db: + with patch(servers_mod) as mock_mod: + _persist_saved_password(server, b'enc-pwd') + + self.assertEqual(server.save_password, 1) + self.assertEqual(server.password, b'enc-pwd') + mock_mod.get_shared_server.assert_not_called() + mock_db.session.commit.assert_called_once() + + +class TestPersistSavedPasswordSharedNonOwner( + _NoServerSetupMixin, BaseTestGenerator): + """A non-owner's rotated password lands on their SharedServer row and + leaves the owner's Server row untouched.""" + + def runTest(self): + owner_server = MagicMock(shared=True, user_id=1, servergroup_id=7) + shared_server = MagicMock() + servers_mod = 'pgadmin.browser.server_groups.servers' + + with patch('pgadmin.model.db') as mock_db: + with patch(f'{servers_mod}.ServerModule') as mock_mod: + with patch(f'{servers_mod}.current_user') as mock_user: + mock_user.id = 2 # not the owner (user_id=1) + mock_mod.get_shared_server.return_value = shared_server + + _persist_saved_password(owner_server, b'enc-pwd') + + mock_mod.get_shared_server.assert_called_once_with( + owner_server, 7) + self.assertEqual(shared_server.save_password, 1) + self.assertEqual(shared_server.password, b'enc-pwd') + # The owner's own row must never be touched for a + # shared connection used by a non-owner. + self.assertNotEqual(owner_server.password, b'enc-pwd') + mock_db.session.commit.assert_called_once() + + +class TestPersistSavedPasswordRollsBackOnFailure( + _NoServerSetupMixin, BaseTestGenerator): + + def runTest(self): + server = MagicMock(shared=False, user_id=1) + + with patch('pgadmin.model.db') as mock_db, \ + patch('pgadmin.browser.server_groups.servers.ServerModule'): + mock_db.session.commit.side_effect = Exception('boom') + + with self.assertRaises(Exception): + _persist_saved_password(server, b'enc-pwd') + + mock_db.session.rollback.assert_called_once() + + +class TestPasswordIsValid(_NoServerSetupMixin, BaseTestGenerator): + + def runTest(self): + manager = MagicMock(db='postgres', user='enterprisedb') + manager.create_connection_string.return_value = 'dsn' + + with self.app.app_context(), \ + patch('psycopg.Connection.connect') as mock_connect: + mock_connect.return_value = MagicMock() + self.assertTrue(_password_is_valid(manager, 'correct-horse')) + + import psycopg + mock_connect.side_effect = psycopg.OperationalError( + 'auth failed') + self.assertFalse(_password_is_valid(manager, 'wrong')) + + +class TestStrToBool(_NoServerSetupMixin, BaseTestGenerator): + """save_password may arrive as a real bool, an int, or one of several + string spellings depending on the client (JSON body vs FormData).""" + + def runTest(self): + for truthy in (True, 1, '1', 'true', 'True', 'on', 'yes'): + self.assertTrue(str_to_bool(truthy), msg=repr(truthy)) + for falsy in (False, 0, '0', 'false', 'False', '', None): + self.assertFalse(str_to_bool(falsy), msg=repr(falsy)) diff --git a/web/pgadmin/utils/__init__.py b/web/pgadmin/utils/__init__.py index 0a57d7e6c0f..524be380dd6 100644 --- a/web/pgadmin/utils/__init__.py +++ b/web/pgadmin/utils/__init__.py @@ -358,6 +358,16 @@ def does_utility_exist(file): return error_msg +TRUTHY_STRING_VALUES = ('true', '1', 'on', 'yes') + + +def str_to_bool(value): + """Normalise a boolean-ish value received from form/JSON request data + (which may arrive as a real bool, an int, or one of several string + spellings depending on the client) into an actual bool.""" + return str(value).lower() in TRUTHY_STRING_VALUES + + def get_server(sid, only_owned=False): """Fetch a server by ID with access check. diff --git a/web/pgadmin/utils/driver/psycopg3/connection.py b/web/pgadmin/utils/driver/psycopg3/connection.py index d07a16cefcd..02e7b3cf2bd 100644 --- a/web/pgadmin/utils/driver/psycopg3/connection.py +++ b/web/pgadmin/utils/driver/psycopg3/connection.py @@ -421,6 +421,14 @@ async def connectdbserver(): if status and is_update_password: manager._update_password(encpass) + # Persist the corrected in-memory password to the Flask + # session. Driver.managers is only an in-process cache, so + # without this a fresh worker process handling a later + # request (e.g. opening the Query Tool, in a multi-worker + # deployment) would restore the stale pre-fix manager from + # the session and lose the corrected password, re-triggering + # the password prompt indefinitely. See issue #10128. + manager.update_session() else: if not self.reconnecting and is_update_password: self.wasConnected = False