From 9ab93cb6e27b26bab258f1776f52908c27ff6d9d Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 13:25:49 +0100 Subject: [PATCH 1/5] fix: honour a shared-server non-owner's own PasswordExecCommand PR #9830 (CVE-2026-7813) unconditionally nulled manager.passexec for non-owners of a shared server, which also made ENABLE_SERVER_PASS_EXEC_CMD meaningless for them even though the SharedServer.passexec_cmd column (added in #9835) already gave each non-owner a place to store their own command. _owner_only_fields additionally blocked the update API from ever writing to it, and the front-end field was already live and unguarded by isShared, so the value was unreachable. Continue to never honour the *owner's* passexec_cmd for a non-owner -- that's the actual privilege-escalation vector the CVE fix closed, since any authenticated user can own a shared server. But a non-owner's own SharedServer.passexec_cmd only ever runs in their own request context, exactly like an owned server's would, so there's no reason to block it. Closes #10249 --- .../browser/server_groups/servers/__init__.py | 10 +++-- .../servers/tests/test_shared_server_unit.py | 15 +++---- web/pgadmin/utils/driver/psycopg3/__init__.py | 42 +++++++++++++++---- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/__init__.py b/web/pgadmin/browser/server_groups/servers/__init__.py index cfcb324c4d8..c2696ceef53 100644 --- a/web/pgadmin/browser/server_groups/servers/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/__init__.py @@ -1007,10 +1007,14 @@ def _set_valid_attr_value(self, gid, data, config_param_map, server, raise CryptKeyMissing # Fields that non-owners must never set on their - # SharedServer — they enable command/SQL execution - # or are owner-level concepts not on SharedServer. + # SharedServer — owner-level concepts not on SharedServer. + # passexec_cmd/passexec_expiration are deliberately NOT + # here: a non-owner may set their own, which only ever runs + # in their own request context (see _shared_server_passexec + # in pgadmin.utils.driver.psycopg3). Only inheriting the + # *owner's* passexec_cmd is blocked, and that is enforced in + # connection_manager(), not here. _owner_only_fields = frozenset({ - 'passexec_cmd', 'passexec_expiration', 'db_res', 'db_res_type', }) diff --git a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py index 49f6a2b93eb..71d427150c5 100644 --- a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py +++ b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py @@ -511,8 +511,8 @@ class TestOwnerOnlyFieldsGuard(BaseTestGenerator): for non-owners.""" scenarios = [ - ('Non-owner cannot set passexec_cmd', - dict(test_method='test_nonowner_passexec_blocked')), + ('Non-owner can set their own passexec_cmd', + dict(test_method='test_nonowner_passexec_allowed')), ('Non-owner cannot set db_res or db_res_type', dict(test_method='test_nonowner_db_res_blocked')), ('Owner can set passexec_cmd', @@ -525,7 +525,7 @@ def runTest(self): @patch(SRV_MODULE + '.get_crypt_key', return_value=(True, b'key')) @patch(SRV_MODULE + '.current_user') - def test_nonowner_passexec_blocked(self, mock_cu, mock_ck): + def test_nonowner_passexec_allowed(self, mock_cu, mock_ck): mock_cu.id = 200 # Non-owner from pgadmin.browser.server_groups.servers import \ ServerNode @@ -536,7 +536,7 @@ def test_nonowner_passexec_blocked(self, mock_cu, mock_ck): node.delete_shared_server = MagicMock() data = { - 'passexec_cmd': '/evil/cmd', + 'passexec_cmd': '/usr/bin/my-own-cmd', 'post_connection_sql': 'SET role reader;', } config_map = { @@ -547,9 +547,10 @@ def test_nonowner_passexec_blocked(self, mock_cu, mock_ck): node._set_valid_attr_value( 1, data, config_map, server, ss) - # passexec_cmd should be blocked for non-owners - self.assertIsNone(ss.passexec_cmd) - # post_connection_sql is allowed for non-owners + # Non-owners may set their own SharedServer.passexec_cmd -- + # only inheriting the *owner's* command is blocked, and + # that's enforced in connection_manager(), not here. + self.assertEqual(ss.passexec_cmd, '/usr/bin/my-own-cmd') self.assertEqual(ss.post_connection_sql, 'SET role reader;') diff --git a/web/pgadmin/utils/driver/psycopg3/__init__.py b/web/pgadmin/utils/driver/psycopg3/__init__.py index 60d7f4c2d54..36d6fb1f66c 100644 --- a/web/pgadmin/utils/driver/psycopg3/__init__.py +++ b/web/pgadmin/utils/driver/psycopg3/__init__.py @@ -23,10 +23,11 @@ from threading import Lock import config -from pgadmin.model import Server +from pgadmin.model import Server, SharedServer from pgadmin.utils.server_access import get_server, \ get_user_server_query from pgadmin.utils.exception import ObjectGone +from pgadmin.utils.passexec import PasswordExec from .keywords import scan_keyword from ..abstract import BaseDriver from .connection import Connection @@ -35,6 +36,27 @@ connection_restore_lock = Lock() +def _shared_server_passexec(server): + """Return a PasswordExec built from a shared-server non-owner's + own SharedServer.passexec_cmd, or None. + + The owner's passexec_cmd is never honoured here for a non-owner + -- see #9830 / CVE-2026-7813, where any user able to own a shared + server could otherwise run an arbitrary command in every other + user's request context. A non-owner's own SharedServer.passexec_cmd + carries no such risk: it only ever runs in that same user's own + request context, exactly like an owned server's passexec_cmd would. + """ + shared_server = SharedServer.query.filter_by( + user_id=current_user.id, osid=server.id).first() + if shared_server is None or not shared_server.passexec_cmd: + return None + return PasswordExec( + shared_server.passexec_cmd, server.host, server.port, + shared_server.username or server.username, + shared_server.passexec_expiration) + + class Driver(BaseDriver): """ class Driver(BaseDriver): @@ -84,12 +106,13 @@ def _restore_connections_from_session(self): for server in servers: manager = managers[str(server.id)] = \ ServerManager(server) - # Suppress passexec for non-owners of shared - # servers — it runs commands on the client - # machine and must not inherit the owner's. + # Never inherit the owner's passexec for + # non-owners of shared servers; only their own + # SharedServer.passexec_cmd, if any. if config.SERVER_MODE and server.shared and \ server.user_id != current_user.id: - manager.passexec = None + manager.passexec = \ + _shared_server_passexec(server) if server.id in session_managers: manager._restore( session_managers[server.id]) @@ -152,12 +175,13 @@ def connection_manager(self, sid=None): # server_data was already access-checked above; # it cannot be None at this point. manager = ServerManager(server_data) - # Suppress passexec for non-owners of shared - # servers — it runs commands on the client machine - # and must not inherit the owner's. + # Never inherit the owner's passexec for non-owners + # of shared servers; only their own + # SharedServer.passexec_cmd, if any. if config.SERVER_MODE and server_data.shared and \ server_data.user_id != current_user.id: - manager.passexec = None + manager.passexec = \ + _shared_server_passexec(server_data) managers[str(sid)] = manager return manager From 13d267eb03eb4bb2db358482f66307f8c33e72eb Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 09:02:18 +0100 Subject: [PATCH 2/5] fix: preserve non-owner passexec through manager.update() manager.update(server) rebuilds the manager from the server object alone, so the manager.passexec = None guards in update() and connect() -- added when the previous commit only nulled it once, on first construction via connection_manager() -- were also firing on every subsequent update()/connect() call and clobbering the non-owner's own passexec right back to None. Recompute it via the same shared_server_passexec() helper connection_manager() already uses instead of blindly nulling it, so it survives. Renamed the helper from _shared_server_passexec to shared_server_passexec since it's now called from browser.server_groups.servers too. Added driver-level unit tests for it, which previously had no direct coverage. Addresses CodeRabbit review on PR #10328. --- .../browser/server_groups/servers/__init__.py | 23 ++-- web/pgadmin/utils/driver/psycopg3/__init__.py | 10 +- .../tests/test_shared_server_passexec.py | 116 ++++++++++++++++++ 3 files changed, 137 insertions(+), 12 deletions(-) create mode 100644 web/pgadmin/utils/driver/psycopg3/tests/test_shared_server_passexec.py diff --git a/web/pgadmin/browser/server_groups/servers/__init__.py b/web/pgadmin/browser/server_groups/servers/__init__.py index c2696ceef53..85c35ce15dd 100644 --- a/web/pgadmin/browser/server_groups/servers/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/__init__.py @@ -30,6 +30,7 @@ from config import PG_DEFAULT_DRIVER from pgadmin.model import db, Server, ServerGroup, User, SharedServer from pgadmin.utils.driver import get_driver +from pgadmin.utils.driver.psycopg3 import shared_server_passexec from pgadmin.utils.master_password import get_crypt_key from pgadmin.utils.exception import CryptKeyMissing, ConnectionLost from pgadmin.tools.schema_diff.node_registry import SchemaDiffRegistry @@ -957,10 +958,12 @@ def update(self, gid, sid): # which will affect the connections. if not conn.connected(): manager.update(server) - # Suppress passexec for non-owners so the manager - # never holds the owner's password-exec command. + # manager.update() rebuilds the manager from the server + # object alone, dropping any passexec. Recompute it so a + # non-owner's own SharedServer.passexec_cmd survives, while + # still never inheriting the owner's. if _is_non_owner(server): - manager.passexec = None + manager.passexec = shared_server_passexec(server) return jsonify( node=self.blueprint.generate_browser_node( @@ -1010,7 +1013,7 @@ def _set_valid_attr_value(self, gid, data, config_param_map, server, # SharedServer — owner-level concepts not on SharedServer. # passexec_cmd/passexec_expiration are deliberately NOT # here: a non-owner may set their own, which only ever runs - # in their own request context (see _shared_server_passexec + # in their own request context (see shared_server_passexec # in pgadmin.utils.driver.psycopg3). Only inheriting the # *owner's* passexec_cmd is blocked, and that is enforced in # connection_manager(), not here. @@ -1632,12 +1635,14 @@ def connect(self, gid, sid, is_qt=False, server=None): # the API call is not made from SQL Editor or View/Edit Data tool if not manager.connection().connected() and not is_qt: manager.update(server) - # Re-suppress passexec after update() which rebuilds - # from the (overlaid) server object. Belt-and-suspenders: - # the overlay already defaults passexec to None, but this - # guards against direct DB edits. + # manager.update() rebuilds the manager from the (overlaid) + # server object alone, dropping any passexec. Recompute it + # so a non-owner's own SharedServer.passexec_cmd survives, + # while still never inheriting the owner's. server.id is + # preserved through the overlay, so this still resolves + # against the right SharedServer row. if _is_non_owner(server): - manager.passexec = None + manager.passexec = shared_server_passexec(server) conn = manager.connection() # Get enc key diff --git a/web/pgadmin/utils/driver/psycopg3/__init__.py b/web/pgadmin/utils/driver/psycopg3/__init__.py index 36d6fb1f66c..12626a021b2 100644 --- a/web/pgadmin/utils/driver/psycopg3/__init__.py +++ b/web/pgadmin/utils/driver/psycopg3/__init__.py @@ -36,7 +36,7 @@ connection_restore_lock = Lock() -def _shared_server_passexec(server): +def shared_server_passexec(server): """Return a PasswordExec built from a shared-server non-owner's own SharedServer.passexec_cmd, or None. @@ -46,6 +46,10 @@ def _shared_server_passexec(server): user's request context. A non-owner's own SharedServer.passexec_cmd carries no such risk: it only ever runs in that same user's own request context, exactly like an owned server's passexec_cmd would. + + Also used outside this module (browser.server_groups.servers) to + recompute passexec after a manager.update() call, which otherwise + rebuilds the manager from the server object alone and drops it. """ shared_server = SharedServer.query.filter_by( user_id=current_user.id, osid=server.id).first() @@ -112,7 +116,7 @@ def _restore_connections_from_session(self): if config.SERVER_MODE and server.shared and \ server.user_id != current_user.id: manager.passexec = \ - _shared_server_passexec(server) + shared_server_passexec(server) if server.id in session_managers: manager._restore( session_managers[server.id]) @@ -181,7 +185,7 @@ def connection_manager(self, sid=None): if config.SERVER_MODE and server_data.shared and \ server_data.user_id != current_user.id: manager.passexec = \ - _shared_server_passexec(server_data) + shared_server_passexec(server_data) managers[str(sid)] = manager return manager diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_shared_server_passexec.py b/web/pgadmin/utils/driver/psycopg3/tests/test_shared_server_passexec.py new file mode 100644 index 00000000000..4abbaf65a8e --- /dev/null +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_shared_server_passexec.py @@ -0,0 +1,116 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Unit tests for shared_server_passexec(), used by connection_manager(), +_restore_connections_from_session(), and by browser.server_groups.servers +to recompute a non-owner's passexec after manager.update() rebuilds the +manager from the server object alone (see #10249). +""" + +from unittest.mock import MagicMock, patch +from pgadmin.utils.route import BaseTestGenerator + +DRIVER_MODULE = 'pgadmin.utils.driver.psycopg3' + + +def _make_server(**overrides): + defaults = dict( + id=1, host='db.owner.com', port=5432, username='owner', + ) + defaults.update(overrides) + server = MagicMock() + for k, v in defaults.items(): + setattr(server, k, v) + return server + + +class TestSharedServerPassexec(BaseTestGenerator): + """Verify shared_server_passexec() resolves a non-owner's own + PasswordExec and never the owner's.""" + + scenarios = [ + ("Non-owner's own passexec_cmd is used", + dict(test_method='test_nonowner_own_cmd_used')), + ('No SharedServer row -> None', + dict(test_method='test_no_shared_server_row')), + ('SharedServer row with no passexec_cmd -> None', + dict(test_method='test_shared_server_no_cmd')), + ("Falls back to owner's server.username when " + 'SharedServer.username is blank', + dict(test_method='test_username_falls_back_to_server')), + ] + + def runTest(self): + getattr(self, self.test_method)() + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + def test_nonowner_own_cmd_used(self, mock_ss_cls, mock_cu): + from pgadmin.utils.driver.psycopg3 import \ + shared_server_passexec + + mock_cu.id = 200 + shared_server = MagicMock( + passexec_cmd='/usr/bin/my-own-cmd', + passexec_expiration=120, username='nonowner') + mock_ss_cls.query.filter_by.return_value \ + .first.return_value = shared_server + + server = _make_server() + result = shared_server_passexec(server) + + mock_ss_cls.query.filter_by.assert_called_once_with( + user_id=200, osid=server.id) + self.assertIsNotNone(result) + self.assertEqual(result.cmd, '/usr/bin/my-own-cmd') + self.assertEqual(result.host, 'db.owner.com') + self.assertEqual(result.port, 5432) + self.assertEqual(result.username, 'nonowner') + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + def test_no_shared_server_row(self, mock_ss_cls, mock_cu): + from pgadmin.utils.driver.psycopg3 import \ + shared_server_passexec + + mock_cu.id = 200 + mock_ss_cls.query.filter_by.return_value \ + .first.return_value = None + + self.assertIsNone(shared_server_passexec(_make_server())) + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + def test_shared_server_no_cmd(self, mock_ss_cls, mock_cu): + from pgadmin.utils.driver.psycopg3 import \ + shared_server_passexec + + mock_cu.id = 200 + shared_server = MagicMock(passexec_cmd=None) + mock_ss_cls.query.filter_by.return_value \ + .first.return_value = shared_server + + self.assertIsNone(shared_server_passexec(_make_server())) + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + def test_username_falls_back_to_server(self, mock_ss_cls, mock_cu): + from pgadmin.utils.driver.psycopg3 import \ + shared_server_passexec + + mock_cu.id = 200 + shared_server = MagicMock( + passexec_cmd='/usr/bin/my-own-cmd', + passexec_expiration=None, username=None) + mock_ss_cls.query.filter_by.return_value \ + .first.return_value = shared_server + + result = shared_server_passexec(_make_server(username='owner')) + + self.assertEqual(result.username, 'owner') From 70182c1f7a6e65404cae7221a73e81396dc799a0 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 10:13:02 +0100 Subject: [PATCH 3/5] fix: refresh live passexec after a connected passexec_cmd change update() skips manager.update(server) while the server is connected (deliberately, so unrelated live-connection state isn't disturbed), but passexec_cmd/passexec_expiration are not in the connected-modify block list, so a change to either was still committed to the DB without ever reaching the manager. Connection.connect() reads manager.passexec lazily, including on an automatic mid-session reconnect (__attempt_execution_reconnect), so it kept using the pre-change command until the next full disconnect/reconnect cycle. Refresh manager.passexec directly in that branch instead, reusing shared_server_passexec() for non-owners and building the PasswordExec inline for owners -- mirrors the not-connected branch without calling the full manager.update(), which would touch other live state. Addresses a second CodeRabbit finding on PR #10328. --- .../browser/server_groups/servers/__init__.py | 15 ++ .../servers/tests/test_shared_server_unit.py | 135 ++++++++++++++++++ 2 files changed, 150 insertions(+) diff --git a/web/pgadmin/browser/server_groups/servers/__init__.py b/web/pgadmin/browser/server_groups/servers/__init__.py index 85c35ce15dd..03a76e5c2f2 100644 --- a/web/pgadmin/browser/server_groups/servers/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/__init__.py @@ -31,6 +31,7 @@ from pgadmin.model import db, Server, ServerGroup, User, SharedServer from pgadmin.utils.driver import get_driver from pgadmin.utils.driver.psycopg3 import shared_server_passexec +from pgadmin.utils.passexec import PasswordExec from pgadmin.utils.master_password import get_crypt_key from pgadmin.utils.exception import CryptKeyMissing, ConnectionLost from pgadmin.tools.schema_diff.node_registry import SchemaDiffRegistry @@ -964,6 +965,20 @@ def update(self, gid, sid): # still never inheriting the owner's. if _is_non_owner(server): manager.passexec = shared_server_passexec(server) + elif 'passexec_cmd' in data or 'passexec_expiration' in data: + # manager.update() is skipped while connected, but a + # changed passexec_cmd/passexec_expiration is still + # committed above. manager.passexec is read lazily on + # the next reconnect (Connection.__attempt_execution_ + # reconnect), so refresh it now or a mid-session + # reconnect would keep using the pre-change command. + if _is_non_owner(server): + manager.passexec = shared_server_passexec(server) + else: + manager.passexec = PasswordExec( + server.passexec_cmd, server.host, server.port, + server.username, server.passexec_expiration) \ + if server.passexec_cmd else None return jsonify( node=self.blueprint.generate_browser_node( diff --git a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py index 71d427150c5..d263314035b 100644 --- a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py +++ b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py @@ -14,10 +14,13 @@ or HTTP infrastructure. """ +import inspect +import json from unittest.mock import MagicMock, patch, call from pgadmin.utils.route import BaseTestGenerator SRV_MODULE = 'pgadmin.browser.server_groups.servers' +DRIVER_MODULE = 'pgadmin.utils.driver.psycopg3' def _make_server(**overrides): @@ -698,3 +701,135 @@ def test_raises_on_none(self, mock_cu, mock_ss): self.assertIn( 'Failed to create shared server', str(ctx.exception)) + + +class TestUpdateRefreshesLivePassexec(BaseTestGenerator): + """Verify ServerNode.update() refreshes manager.passexec when + passexec_cmd/passexec_expiration changes on a *connected* server. + + manager.update(server) is skipped while connected (it would touch + live connection state), so without an explicit refresh here the + manager keeps serving the pre-change command to + Connection.__attempt_execution_reconnect() on the next automatic + reconnect (CodeRabbit finding on PR #10328). + """ + + scenarios = [ + ("Owner: passexec_cmd change while connected refreshes " + 'the manager', + dict(test_method='test_owner_connected_passexec_refreshed')), + ("Non-owner: passexec_cmd change while connected refreshes " + "the manager from their own SharedServer row", + dict(test_method='test_nonowner_connected_passexec_refreshed')), + ('Unrelated field change while connected leaves passexec ' + 'untouched', + dict(test_method='test_connected_unrelated_field_untouched')), + ] + + def runTest(self): + getattr(self, self.test_method)() + + def _call_update( + self, server, data, connected, manager, sharedserver=None): + """Invoke the undecorated ServerNode.update(gid, sid) with + collaborators mocked, and return the manager it acted on.""" + from pgadmin.browser.server_groups.servers import ServerNode + + driver = MagicMock() + driver.connection_manager.return_value = manager + manager.connection.return_value.connected.return_value = connected + manager.user_info = {} + + node = ServerNode.__new__(ServerNode) + node.blueprint = MagicMock() + node.blueprint.generate_browser_node.return_value = {} + node.node_type = 'server' + node.delete_shared_server = MagicMock() + + with self.app.test_request_context( + '/', method='PUT', data=json.dumps(data), + content_type='application/json'), \ + patch(SRV_MODULE + '.get_server', return_value=server), \ + patch(SRV_MODULE + '.get_driver', return_value=driver), \ + patch(SRV_MODULE + '.get_crypt_key', + return_value=(True, b'key')), \ + patch(SRV_MODULE + '.db'), \ + patch(SRV_MODULE + '.jsonify', side_effect=lambda **kw: kw), \ + patch.object( + __import__(SRV_MODULE, fromlist=['ServerModule']). + ServerModule, 'get_shared_server', + return_value=sharedserver): + raw_update = inspect.unwrap(ServerNode.update) + raw_update(node, 1, 1) + + return manager + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + @patch(SRV_MODULE + '.current_user') + @patch(SRV_MODULE + '.config') + def test_owner_connected_passexec_refreshed( + self, mock_config, mock_cu, mock_driver_ss, mock_driver_cu): + mock_config.SERVER_MODE = True + mock_cu.id = 100 # Owner + + server = _make_server() + manager = MagicMock() + data = {'passexec_cmd': '/usr/bin/new-owner-cmd', + 'passexec_expiration': 90} + + manager = self._call_update( + server, data, connected=True, manager=manager) + + self.assertIsNotNone(manager.passexec) + self.assertEqual(manager.passexec.cmd, '/usr/bin/new-owner-cmd') + self.assertEqual(manager.passexec.expiration_seconds, 90) + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + @patch(SRV_MODULE + '.current_user') + @patch(SRV_MODULE + '.config') + def test_nonowner_connected_passexec_refreshed( + self, mock_config, mock_cu, mock_driver_ss, mock_driver_cu): + mock_config.SERVER_MODE = True + mock_cu.id = 200 # Non-owner + mock_driver_cu.id = 200 + + server = _make_server() + ss = _make_shared_server() + # The driver's own SharedServer query must resolve to the + # same ss instance _set_valid_attr_value() just wrote to. + mock_driver_ss.query.filter_by.return_value.first.return_value = ss + + manager = MagicMock() + data = {'passexec_cmd': '/usr/bin/my-own-cmd', + 'passexec_expiration': 60} + + manager = self._call_update( + server, data, connected=True, manager=manager, + sharedserver=ss) + + self.assertEqual(ss.passexec_cmd, '/usr/bin/my-own-cmd') + self.assertIsNotNone(manager.passexec) + self.assertEqual(manager.passexec.cmd, '/usr/bin/my-own-cmd') + + @patch(DRIVER_MODULE + '.current_user') + @patch(DRIVER_MODULE + '.SharedServer') + @patch(SRV_MODULE + '.current_user') + @patch(SRV_MODULE + '.config') + def test_connected_unrelated_field_untouched( + self, mock_config, mock_cu, mock_driver_ss, mock_driver_cu): + mock_config.SERVER_MODE = True + mock_cu.id = 100 # Owner + + server = _make_server() + manager = MagicMock() + sentinel = manager.passexec # whatever the manager already has + data = {'name': 'NewName'} + + manager = self._call_update( + server, data, connected=True, manager=manager) + + # No passexec field changed -- manager.passexec must be left + # exactly as it was, not recomputed or cleared. + self.assertIs(manager.passexec, sentinel) From 10388ed27dc4a83fb4d8cffd390af83a7052fd51 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 13:01:04 +0100 Subject: [PATCH 4/5] test: assert on update()'s return value, not just manager state _call_update() discarded raw_update()'s return value, so a test could pass vacuously if update() took an early return (e.g. its "no parameters were changed" guard) before ever reaching the passexec-refresh branch under test -- manager is a MagicMock and wouldn't itself flag that. Return (manager, result) and assert 'node' in result in each test to confirm the success path was actually reached. Addresses a third CodeRabbit finding on PR #10328. --- .../servers/tests/test_shared_server_unit.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py index d263314035b..515fd257e61 100644 --- a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py +++ b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py @@ -732,7 +732,11 @@ def runTest(self): def _call_update( self, server, data, connected, manager, sharedserver=None): """Invoke the undecorated ServerNode.update(gid, sid) with - collaborators mocked, and return the manager it acted on.""" + collaborators mocked. Returns (manager, result) -- callers + must assert on result too, so a test can't pass vacuously + by way of update() returning early (e.g. its "no parameters + were changed" guard) before ever reaching the code under + test.""" from pgadmin.browser.server_groups.servers import ServerNode driver = MagicMock() @@ -760,9 +764,9 @@ def _call_update( ServerModule, 'get_shared_server', return_value=sharedserver): raw_update = inspect.unwrap(ServerNode.update) - raw_update(node, 1, 1) + result = raw_update(node, 1, 1) - return manager + return manager, result @patch(DRIVER_MODULE + '.current_user') @patch(DRIVER_MODULE + '.SharedServer') @@ -778,9 +782,10 @@ def test_owner_connected_passexec_refreshed( data = {'passexec_cmd': '/usr/bin/new-owner-cmd', 'passexec_expiration': 90} - manager = self._call_update( + manager, result = self._call_update( server, data, connected=True, manager=manager) + self.assertIn('node', result) self.assertIsNotNone(manager.passexec) self.assertEqual(manager.passexec.cmd, '/usr/bin/new-owner-cmd') self.assertEqual(manager.passexec.expiration_seconds, 90) @@ -805,10 +810,11 @@ def test_nonowner_connected_passexec_refreshed( data = {'passexec_cmd': '/usr/bin/my-own-cmd', 'passexec_expiration': 60} - manager = self._call_update( + manager, result = self._call_update( server, data, connected=True, manager=manager, sharedserver=ss) + self.assertIn('node', result) self.assertEqual(ss.passexec_cmd, '/usr/bin/my-own-cmd') self.assertIsNotNone(manager.passexec) self.assertEqual(manager.passexec.cmd, '/usr/bin/my-own-cmd') @@ -827,9 +833,10 @@ def test_connected_unrelated_field_untouched( sentinel = manager.passexec # whatever the manager already has data = {'name': 'NewName'} - manager = self._call_update( + manager, result = self._call_update( server, data, connected=True, manager=manager) + self.assertIn('node', result) # No passexec field changed -- manager.passexec must be left # exactly as it was, not recomputed or cleared. self.assertIs(manager.passexec, sentinel) From 3a8492ecfcf06be9c674171df28f4793c8074bd5 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 14:04:21 +0100 Subject: [PATCH 5/5] test: assert passexec_expiration too, not just passexec_cmd test_nonowner_connected_passexec_refreshed sent passexec_expiration=60 in the update payload but only asserted on passexec_cmd afterwards, so a regression that persisted/refreshed the command but dropped the expiration would have passed unnoticed. Addresses a fourth CodeRabbit finding on PR #10328. --- .../server_groups/servers/tests/test_shared_server_unit.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py index 515fd257e61..5e2feb9894c 100644 --- a/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py +++ b/web/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.py @@ -816,8 +816,10 @@ def test_nonowner_connected_passexec_refreshed( self.assertIn('node', result) self.assertEqual(ss.passexec_cmd, '/usr/bin/my-own-cmd') + self.assertEqual(ss.passexec_expiration, 60) self.assertIsNotNone(manager.passexec) self.assertEqual(manager.passexec.cmd, '/usr/bin/my-own-cmd') + self.assertEqual(manager.passexec.expiration_seconds, 60) @patch(DRIVER_MODULE + '.current_user') @patch(DRIVER_MODULE + '.SharedServer')