diff --git a/web/pgadmin/browser/server_groups/servers/__init__.py b/web/pgadmin/browser/server_groups/servers/__init__.py index e0161ca2b52..9e23d8343b6 100644 --- a/web/pgadmin/browser/server_groups/servers/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/__init__.py @@ -46,7 +46,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, \ get_invalid_connection_param_key, ServiceNotAllowed, \ check_connection_params_allowed from pgadmin.settings.utils import with_object_filters @@ -1836,6 +1836,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 @@ -1904,8 +1910,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 @@ -1956,6 +1969,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) @@ -1973,6 +1992,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: @@ -2397,6 +2438,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 @@ -2419,6 +2461,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..e9a9c9cb244 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,14 @@ export default function ConnectServerContent({closeModal, data, onOK, setHeight, } if(data.prompt_password) { postFormData.append('password', formData.password); - formData.save_password && + // Send the checkbox state when the server told us the + // current setting (so an explicit uncheck is distinguishable + // from "field not sent"), or when the user ticked it. A + // prompt that doesn't report save_password must never send + // false, as that would clear an existing saved password. + if(data.save_password !== undefined || formData.save_password) { postFormData.append('save_password', formData.save_password); + } } onOK?.(postFormData); closeModal(); diff --git a/web/pgadmin/tools/erd/__init__.py b/web/pgadmin/tools/erd/__init__.py index 52c7333c179..e488b54405a 100644 --- a/web/pgadmin/tools/erd/__init__.py +++ b/web/pgadmin/tools/erd/__init__.py @@ -607,6 +607,10 @@ def initialize_erd(trans_id, sgid, sid, did): try: conn = _get_connection(sid, did, trans_id, data.get('db_name', None)) except ConnectionLost as e: + # Report the saved-password state so the password prompt's + # "Save Password" checkbox reflects the record that will be updated. + from pgadmin.tools.sqleditor import _get_save_password_target + erd_server = get_server(sid) return make_json_response( success=0, status=428, @@ -615,6 +619,9 @@ def initialize_erd(trans_id, sgid, sid, did): "server_type":data.get('server_type', None), "errmsg": str(e), "prompt_password": True, + "save_password": bool( + erd_server is not None and + _get_save_password_target(erd_server).save_password), "allow_save_password": True if ALLOW_SAVE_PASSWORD and session.get('allow_save_password', None) else False, diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index 8fa4c9204e2..08517041d7e 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -43,7 +43,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 @@ -296,6 +296,9 @@ 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( + _get_save_password_target( + server).save_password), "allow_save_password": True if ALLOW_SAVE_PASSWORD and session.get('allow_save_password', None) @@ -648,6 +651,9 @@ 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( + _get_save_password_target( + server).save_password), "allow_save_password": True if ALLOW_SAVE_PASSWORD and session.get('allow_save_password', None) @@ -2915,7 +2921,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."), @@ -2961,7 +2967,7 @@ def _connects_as_server_user(server): role in (None, server.role or None) -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 @@ -2972,6 +2978,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. @@ -2992,12 +3005,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..b3be3474b78 --- /dev/null +++ b/web/pgadmin/tools/sqleditor/tests/test_persist_saved_password.py @@ -0,0 +1,206 @@ +########################################################################## +# +# 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 +import pgadmin.tools.sqleditor as sqleditor +from pgadmin.tools.sqleditor import _persist_saved_password, \ + _password_is_valid, _clear_saved_password + + +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)) + + +class TestClearSavedPasswordOwner(_NoServerSetupMixin, BaseTestGenerator): + """An explicit opt-out clears the owned Server row's saved password.""" + + def runTest(self): + server = MagicMock(shared=False, user_id=1, save_password=1, + password=b'old') + + with patch('pgadmin.model.db') as mock_db, \ + patch('pgadmin.browser.server_groups.servers.ServerModule'): + _clear_saved_password(server) + + self.assertEqual(server.save_password, 0) + self.assertIsNone(server.password) + mock_db.session.commit.assert_called_once() + + +class TestClearSavedPasswordSharedNonOwner( + _NoServerSetupMixin, BaseTestGenerator): + """A non-owner's opt-out clears 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, + save_password=1, password=b'owner-enc') + shared_server = MagicMock(save_password=1, password=b'shared-enc') + 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 + mock_mod.get_shared_server.return_value = shared_server + + _clear_saved_password(owner_server) + + self.assertEqual(shared_server.save_password, 0) + self.assertIsNone(shared_server.password) + self.assertEqual(owner_server.save_password, 1) + self.assertEqual(owner_server.password, b'owner-enc') + mock_db.session.commit.assert_called_once() + + +class TestCachePasswordSaveFlagHandling( + _NoServerSetupMixin, BaseTestGenerator): + """save_password=false clears a saved password, save_password=true + persists the new one, and a request that omits the field must leave a + saved password alone.""" + + scenarios = [ + ('Explicit false clears the saved password', dict( + form={'password': 'pw', 'save_password': 'false'}, + expect_persist=False, expect_clear=True)), + ('Explicit true persists the password', dict( + form={'password': 'pw', 'save_password': 'true'}, + expect_persist=True, expect_clear=False)), + ('Missing field leaves the saved password alone', dict( + form={'password': 'pw'}, + expect_persist=False, expect_clear=False)), + ] + + def runTest(self): + manager = MagicMock() + server = MagicMock() + mock_request = MagicMock() + mock_request.form = self.form + + with patch.object(sqleditor, 'request', mock_request), \ + patch.object(sqleditor, 'current_app', MagicMock()), \ + patch.object(sqleditor, 'ALLOW_SAVE_PASSWORD', True), \ + patch.object(sqleditor, 'get_crypt_key', + return_value=(True, 'key')), \ + patch.object(sqleditor, '_password_is_valid', + return_value=True), \ + patch.object(sqleditor, + '_persist_saved_password') as persist, \ + patch.object(sqleditor, + '_clear_saved_password') as clear: + sqleditor._cache_manager_password_from_request(manager, server) + + self.assertEqual(persist.called, self.expect_persist) + self.assertEqual(clear.called, self.expect_clear) diff --git a/web/pgadmin/tools/sqleditor/utils/tests/test_cache_manager_password.py b/web/pgadmin/tools/sqleditor/utils/tests/test_cache_manager_password.py index 7491883079f..9e25494d7d1 100644 --- a/web/pgadmin/tools/sqleditor/utils/tests/test_cache_manager_password.py +++ b/web/pgadmin/tools/sqleditor/utils/tests/test_cache_manager_password.py @@ -61,6 +61,12 @@ class CacheManagerPasswordTest(BaseTestGenerator): crypt_key_present=False, expect_cached=False, )), + ('When the password fails validation it is not cached', dict( + form_data={'password': LONG_TOKEN}, + crypt_key_present=True, + password_valid=False, + expect_cached=False, + )), ('When the request body is malformed JSON it is a silent no-op', dict( form_data={}, request_data=b'{not-valid-json', @@ -86,6 +92,8 @@ def _update_password(passwd): with patch.object(sqleditor, 'request', mock_request), \ patch.object(sqleditor, 'current_app', MagicMock()), \ + patch.object(sqleditor, '_password_is_valid', + return_value=getattr(self, 'password_valid', True)), \ patch.object(sqleditor, 'get_crypt_key', return_value=(self.crypt_key_present, crypt_key)): sqleditor._cache_manager_password_from_request(manager) diff --git a/web/pgadmin/utils/__init__.py b/web/pgadmin/utils/__init__.py index 43a5e82f500..6143b8c1bcb 100644 --- a/web/pgadmin/utils/__init__.py +++ b/web/pgadmin/utils/__init__.py @@ -935,6 +935,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 10be01d79d7..89fa8254965 100644 --- a/web/pgadmin/utils/driver/psycopg3/connection.py +++ b/web/pgadmin/utils/driver/psycopg3/connection.py @@ -649,6 +649,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 diff --git a/web/regression/javascript/Dialogs/ConnectServerContent.spec.js b/web/regression/javascript/Dialogs/ConnectServerContent.spec.js new file mode 100644 index 00000000000..96403a07eef --- /dev/null +++ b/web/regression/javascript/Dialogs/ConnectServerContent.spec.js @@ -0,0 +1,50 @@ +///////////////////////////////////////////////////////////// +// +// pgAdmin 4 - PostgreSQL Tools +// +// Copyright (C) 2013 - 2026, The pgAdmin Development Team +// This software is released under the PostgreSQL Licence +// +////////////////////////////////////////////////////////////// + +import { render, screen, fireEvent } from '@testing-library/react'; +import { withTheme } from '../fake_theme'; +import ConnectServerContent from '../../../pgadmin/static/js/Dialogs/ConnectServerContent'; + +const Themed = withTheme(ConnectServerContent); + +function submit(data, {tick=false}={}) { + const onOK = jest.fn(); + render(); + if(tick) { + fireEvent.click(screen.getByRole('checkbox')); + } + fireEvent.click(document.querySelector('[data-test="save"]')); + return onOK.mock.calls[0][0]; +} + +describe('ConnectServerContent save_password', () => { + it('sends false when the server reported save_password and it was unticked', () => { + const formData = submit({save_password: false}); + expect(formData.get('save_password')).toBe('false'); + }); + + it('seeds the checkbox from the server setting and sends true', () => { + const formData = submit({save_password: true}); + expect(formData.get('save_password')).toBe('true'); + }); + + it('does not send the field when the prompt did not report it and it is unticked', () => { + const formData = submit({}); + expect(formData.has('save_password')).toBe(false); + }); + + it('sends true when the prompt did not report it but the user ticked the box', () => { + const formData = submit({}, {tick: true}); + expect(formData.get('save_password')).toBe('true'); + }); +});