From bd252cae30bc371064b50f8e7dcec46909e571b0 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 11:30:20 +0100 Subject: [PATCH 1/6] fix: correct existingSecret path in Helm deployment template (#10214) The annotation conditions referenced the non-existent top-level .Values.existingSecret instead of .Values.auth.existingSecret, so the secret checksum annotation and the empty-secret gating never worked as intended when an existing secret wasn't supplied. --- pkg/helm/templates/deployment.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/helm/templates/deployment.yaml b/pkg/helm/templates/deployment.yaml index 5c12294868b..854e809180c 100644 --- a/pkg/helm/templates/deployment.yaml +++ b/pkg/helm/templates/deployment.yaml @@ -32,12 +32,12 @@ spec: {{- with omit .Values.commonLabels "app" }} {{- . | toYaml | nindent 8 }} {{- end }} - {{- if or (not (empty .Values.commonAnnotations)) (not .Values.existingSecret) .Values.preferences.enabled .Values.serverDefinitions.enabled }} + {{- if or (not (empty .Values.commonAnnotations)) (empty .Values.auth.existingSecret) .Values.preferences.enabled .Values.serverDefinitions.enabled }} annotations: {{- with .Values.commonAnnotations }} {{- . | toYaml | nindent 8 }} {{- end }} - {{- if not .Values.existingSecret }} + {{- if empty .Values.auth.existingSecret }} checksum/secret: {{ include (print $.Template.BasePath "/secret.yaml") . | sha256sum }} {{- end }} {{- if and .Values.config_local.enabled (empty .Values.config_local.existingSecret) }} From 1f8a0755354884520e750f202fc75d385b33a403 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 11:32:24 +0100 Subject: [PATCH 2/6] fix: place CONCURRENTLY correctly in generated REINDEX SQL (#10251) CONCURRENTLY was being appended to the parenthesised option list alongside VERBOSE etc., which PostgreSQL rejects. It's not a parenthesizable option: it belongs standalone, between the object type keyword and the object name. --- .../tools/maintenance/templates/maintenance/sql/command.sql | 5 ++--- .../tests/test_maintenance_create_job_unit_test.py | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/web/pgadmin/tools/maintenance/templates/maintenance/sql/command.sql b/web/pgadmin/tools/maintenance/templates/maintenance/sql/command.sql index 32abb115f71..35312924227 100644 --- a/web/pgadmin/tools/maintenance/templates/maintenance/sql/command.sql +++ b/web/pgadmin/tools/maintenance/templates/maintenance/sql/command.sql @@ -14,7 +14,6 @@ {% if data.vacuum_parallel %}{{ maintenance_options.append('PARALLEL ' + data.vacuum_parallel) or "" }}{% endif %} {% if data.buffer_usage_limit %}{{ maintenance_options.append('BUFFER_USAGE_LIMIT "' + data.buffer_usage_limit + '"') or "" }}{% endif %} {% if data.reindex_tablespace %}{{ maintenance_options.append('TABLESPACE ' + conn|qtIdent(data.reindex_tablespace)) or "" }}{% endif %} -{% if data.reindex_concurrently %}{{ maintenance_options.append('CONCURRENTLY') or "" }}{% endif %} {% if data.op == "VACUUM" %} VACUUM{% for option in maintenance_options %}{% if loop.first %} ({% endif %}{{ option }}{% if not loop.last %}, {% endif %}{% if loop.last %}){% endif %}{% endfor %}{% if data.schema %} {{ conn|qtIdent(data.schema) }}.{{ conn|qtIdent(data.table) }}{% endif %}; {% endif %} @@ -23,9 +22,9 @@ ANALYZE{% for option in maintenance_options %}{% if loop.first %} ({% endif %}{{ {% endif %} {% if data.op == "REINDEX" %} {% if index_name %} -REINDEX{% for option in maintenance_options %}{% if loop.first %} ({% endif %}{{ option }}{% if not loop.last %}, {% endif %}{% if loop.last %}){% endif %}{% endfor %} INDEX {{ conn|qtIdent(data.schema, index_name) }}; +REINDEX{% for option in maintenance_options %}{% if loop.first %} ({% endif %}{{ option }}{% if not loop.last %}, {% endif %}{% if loop.last %}){% endif %}{% endfor %} INDEX{% if data.reindex_concurrently %} CONCURRENTLY{% endif %} {{ conn|qtIdent(data.schema, index_name) }}; {% else %} -REINDEX{% for option in maintenance_options %}{% if loop.first %} ({% endif %}{{ option }}{% if not loop.last %}, {% endif %}{% if loop.last %}){% endif %}{% endfor %}{% if not data.schema and not data.reindex_system %} DATABASE {{ conn|qtIdent(data.database) }}{% elif not data.schema and data.reindex_system%} SYSTEM {{ conn|qtIdent(data.database) }}{% elif data.schema and not data.table and not data.primary_key and not data.unique_constraint and not data.index and not data.mview %} SCHEMA {{ conn|qtIdent(data.schema) }}{% else %} TABLE {{ conn|qtIdent(data.schema, data.table) }}{% endif %}; +REINDEX{% for option in maintenance_options %}{% if loop.first %} ({% endif %}{{ option }}{% if not loop.last %}, {% endif %}{% if loop.last %}){% endif %}{% endfor %}{% if not data.schema and not data.reindex_system %} DATABASE{% if data.reindex_concurrently %} CONCURRENTLY{% endif %} {{ conn|qtIdent(data.database) }}{% elif not data.schema and data.reindex_system%} SYSTEM {{ conn|qtIdent(data.database) }}{% elif data.schema and not data.table and not data.primary_key and not data.unique_constraint and not data.index and not data.mview %} SCHEMA{% if data.reindex_concurrently %} CONCURRENTLY{% endif %} {{ conn|qtIdent(data.schema) }}{% else %} TABLE{% if data.reindex_concurrently %} CONCURRENTLY{% endif %} {{ conn|qtIdent(data.schema, data.table) }}{% endif %}; {% endif %} {% endif %} {% if data.op == "CLUSTER" %} diff --git a/web/pgadmin/tools/maintenance/tests/test_maintenance_create_job_unit_test.py b/web/pgadmin/tools/maintenance/tests/test_maintenance_create_job_unit_test.py index 0b6f265b9ee..54fdfebc670 100644 --- a/web/pgadmin/tools/maintenance/tests/test_maintenance_create_job_unit_test.py +++ b/web/pgadmin/tools/maintenance/tests/test_maintenance_create_job_unit_test.py @@ -537,7 +537,7 @@ class MaintenanceCreateJobTest(BaseTestGenerator): verbose=True ), url=MAINTENANCE_URL, - expected_cmd_opts=['REINDEX (VERBOSE, CONCURRENTLY) DATABASE ' + expected_cmd_opts=['REINDEX (VERBOSE) DATABASE CONCURRENTLY ' 'postgres;\n'], server_min_version=120000, message='REINDEX CONCURRENTLY is not supported by EPAS/PG server ' @@ -643,7 +643,7 @@ class MaintenanceCreateJobTest(BaseTestGenerator): verbose=True ), url=MAINTENANCE_URL, - expected_cmd_opts=['REINDEX (VERBOSE, CONCURRENTLY) TABLE ' + expected_cmd_opts=['REINDEX (VERBOSE) TABLE CONCURRENTLY ' 'my_schema.my_table;\n'], server_min_version=120000, message='REINDEX CONCURRENTLY TABLE is not supported by ' @@ -710,7 +710,7 @@ class MaintenanceCreateJobTest(BaseTestGenerator): verbose=True ), url=MAINTENANCE_URL, - expected_cmd_opts=['REINDEX (VERBOSE, CONCURRENTLY) INDEX ' + expected_cmd_opts=['REINDEX (VERBOSE) INDEX CONCURRENTLY ' 'my_schema.my_index;\n'], server_min_version=120000, message='REINDEX CONCURRENTLY is not supported by EPAS/PG server ' From 4173ddf0f3bff44b08eb71a65bedbe371680d350 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 11:47:30 +0100 Subject: [PATCH 3/6] fix: allow ADMIN OPTION holders to manage Group Role membership (#9450) A role's membership tab only enabled the add/remove member controls for superusers and CREATEROLE holders, so a user who was themselves granted ADMIN OPTION on that role (and can therefore GRANT/REVOKE its membership at the SQL level) had no way to add other members, and hit a permission error server-side if they tried anyway. The role UI schema now also allows membership changes when the current user is a member of the role with admin=true. The backend mirrors this: permission.sql reports whether the connecting user holds ADMIN OPTION on the target role, and the update handler lets such a request through only when it's restricted to rolmembers changes, so this can't be used to escalate other role attributes. --- .../server_groups/servers/roles/__init__.py | 32 ++++++++-- .../servers/roles/static/js/role.ui.js | 16 ++++- .../roles/sql/default/permission.sql | 11 +++- .../test_role_check_permission_unit_test.py | 62 +++++++++++++++++++ .../schema_ui_files/role.ui.spec.js | 27 ++++++++ 5 files changed, 140 insertions(+), 8 deletions(-) create mode 100644 web/pgadmin/browser/server_groups/servers/roles/tests/test_role_check_permission_unit_test.py diff --git a/web/pgadmin/browser/server_groups/servers/roles/__init__.py b/web/pgadmin/browser/server_groups/servers/roles/__init__.py index a2f407cda68..63a21d7887e 100644 --- a/web/pgadmin/browser/server_groups/servers/roles/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/roles/__init__.py @@ -619,6 +619,7 @@ def _check_action(action, kwargs): return fetch_name, check_permission, forbidden_msg def _check_permission(self, check_permission, action, kwargs): + self.membership_only_update = False if check_permission: user = self.manager.user_info @@ -627,6 +628,15 @@ def _check_permission(self, check_permission, action, kwargs): (action != 'update' or 'rid' in kwargs) and \ kwargs['rid'] != -1 and \ user['id'] != kwargs['rid']: + # A role that only has ADMIN OPTION on this specific role + # (rather than being a superuser or having CREATEROLE) may + # still manage that role's membership, so don't forbid the + # request outright; the update handler restricts what such + # a request is allowed to change to membership only. + if action == 'update' and getattr( + self, 'has_admin_option', False): + self.membership_only_update = True + return False return True return False @@ -658,6 +668,7 @@ def _check_and_fetch_name(self, fetch_name, kwargs): self.role = row['rolname'] self.rolCanLogin = row['rolcanlogin'] self.rolSuper = row['rolsuper'] + self.has_admin_option = row.get('has_admin_option', False) return False, '' @@ -713,16 +724,20 @@ def wrapped(self, **kwargs): fetch_name, check_permission, \ forbidden_msg = RoleView._check_action(action, kwargs) - is_permission_error = self._check_permission(check_permission, - action, kwargs) - if is_permission_error: - return forbidden(forbidden_msg) - + # Fetched first: the permission check needs to know + # whether the current user holds ADMIN OPTION on this + # role before it can decide whether to forbid the + # request. is_error, errmsg = self._check_and_fetch_name(fetch_name, kwargs) if is_error: return errmsg + is_permission_error = self._check_permission(check_permission, + action, kwargs) + if is_permission_error: + return forbidden(forbidden_msg) + return f(self, **kwargs) return wrapped @@ -1023,6 +1038,13 @@ def create(self, gid, sid): @check_precondition(action='update') @validate_request def update(self, gid, sid, rid): + if getattr(self, 'membership_only_update', False) and \ + not set(self.request) <= {'rolmembers'}: + return forbidden( + _("The current user does not have permission to update " + "the role. Users with ADMIN OPTION on this role may " + "only manage its membership.") + ) sql = render_template( self.sql_path + self._UPDATE_SQL, diff --git a/web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js b/web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js index 68ff085dacd..b23f06e0640 100644 --- a/web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js +++ b/web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js @@ -55,6 +55,18 @@ export default class RoleSchema extends BaseUISchema { return (!(user.is_superuser || user.can_create_role) && user.id != state.oid); } + // A role that isn't a superuser or CREATEROLE holder can still manage + // this role's membership if they hold ADMIN OPTION on it themselves. + isMemberAdmin(state) { + return (state.rolmembers ?? []).some( + (member) => member.role === this.user.name && member.admin + ); + } + + membersReadOnly(state) { + return this.readOnly(state) && !this.isMemberAdmin(state); + } + memberDataFormatter(rawData) { let members = ''; if(_.isObject(rawData)) { @@ -194,8 +206,8 @@ export default class RoleSchema extends BaseUISchema { mode: ['edit', 'create'], cell: 'text', type: 'collection', schema: obj.membershipSchema, - disabled: obj.readOnly, - canDelete: (state) => !obj.readOnly(state), + disabled: (state) => obj.membersReadOnly(state), + canDelete: (state) => !obj.membersReadOnly(state), canDeleteRow: true, helpMessage: obj.isReadOnly ? gettext('Select the checkbox for roles to include WITH ADMIN OPTION.') : gettext('Roles shown with a check mark have the WITH ADMIN OPTION set.'), }, diff --git a/web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/permission.sql b/web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/permission.sql index 66b931cd970..7f3febc6645 100644 --- a/web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/permission.sql +++ b/web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/permission.sql @@ -1,5 +1,14 @@ SELECT - rolname, rolcanlogin, rolsuper + rolname, rolcanlogin, rolsuper, + EXISTS ( + SELECT 1 FROM pg_catalog.pg_auth_members am + WHERE am.roleid = {{ rid }}::OID + AND am.member = ( + SELECT oid FROM pg_catalog.pg_roles + WHERE rolname = current_user + ) + AND am.admin_option + ) AS has_admin_option FROM pg_catalog.pg_roles WHERE oid = {{ rid }}::OID diff --git a/web/pgadmin/browser/server_groups/servers/roles/tests/test_role_check_permission_unit_test.py b/web/pgadmin/browser/server_groups/servers/roles/tests/test_role_check_permission_unit_test.py new file mode 100644 index 00000000000..4319ec9ff4d --- /dev/null +++ b/web/pgadmin/browser/server_groups/servers/roles/tests/test_role_check_permission_unit_test.py @@ -0,0 +1,62 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +from unittest.mock import MagicMock + +from pgadmin.utils.route import BaseTestGenerator +from pgadmin.browser.server_groups.servers.roles import RoleView + + +class RoleCheckPermissionTest(BaseTestGenerator): + """Unit tests for RoleView._check_permission's ADMIN OPTION carve-out. + + A role holder who is neither a superuser nor a CREATEROLE holder, but + who has been granted ADMIN OPTION on the specific role being updated, + should be allowed through the permission gate so they can manage that + role's membership - but only for 'update', never for 'drop', and the + view should record that the request must be restricted to membership + changes only. + """ + scenarios = [ + ('Check Role Node', dict(url='/browser/role/obj/')) + ] + + def setUp(self): + pass + + def runTest(self): + view = RoleView(cmd=None) + view.manager = MagicMock() + + # Plain user, no admin option: update is forbidden. + view.manager.user_info = { + 'is_superuser': False, 'can_create_role': False, 'id': 5 + } + view.has_admin_option = False + self.assertTrue(view._check_permission(True, 'update', {'rid': 10})) + self.assertFalse(view.membership_only_update) + + # Same user, but with ADMIN OPTION on the target role: allowed + # through, flagged as membership-only. + view.has_admin_option = True + self.assertFalse(view._check_permission(True, 'update', {'rid': 10})) + self.assertTrue(view.membership_only_update) + + # ADMIN OPTION does not extend to dropping the role. + self.assertTrue(view._check_permission(True, 'drop', {'rid': 10})) + + # Superusers are unaffected by the ADMIN OPTION check. + view.manager.user_info = { + 'is_superuser': True, 'can_create_role': False, 'id': 5 + } + view.has_admin_option = False + self.assertFalse(view._check_permission(True, 'update', {'rid': 10})) + + def tearDown(self): + pass diff --git a/web/regression/javascript/schema_ui_files/role.ui.spec.js b/web/regression/javascript/schema_ui_files/role.ui.spec.js index 63bc47fda6d..7760ac71baf 100644 --- a/web/regression/javascript/schema_ui_files/role.ui.spec.js +++ b/web/regression/javascript/schema_ui_files/role.ui.spec.js @@ -45,5 +45,32 @@ describe('RoleSchema', ()=>{ it('properties', async ()=>{ await getPropertiesView(createSchemaObject(), getInitData); }); + + describe('membersReadOnly', ()=>{ + it('is read only for a plain user who is not an admin member', ()=>{ + const schemaObj = createSchemaObject(); + const state = {oid: 123, rolmembers: [{role: 'postgres', admin: false}]}; + expect(schemaObj.membersReadOnly(state)).toBe(true); + }); + + it('is editable for a user with ADMIN OPTION on the role', ()=>{ + const schemaObj = createSchemaObject(); + const state = {oid: 123, rolmembers: [{role: 'postgres', admin: true}]}; + expect(schemaObj.membersReadOnly(state)).toBe(false); + }); + + it('is editable regardless when the user is a superuser/can create roles', ()=>{ + const schemaObj = new RoleSchema( + ()=>new MockSchema(), + ()=>new MockSchema(), + { + role: ()=>[], + nodeInfo: {server: {user: {name: 'postgres', id: 0, is_superuser: true}}} + }, + ); + const state = {oid: 123, rolmembers: []}; + expect(schemaObj.membersReadOnly(state)).toBe(false); + }); + }); }); From 0713356dbe825646f4f4e16ba6adb42575bb8be1 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 12:20:10 +0100 Subject: [PATCH 4/6] fix: reject empty or null Username on non-shared server import (#10309) validate_json_data() only checked that the Username key was present on a non-shared server, not that it held anything useful, so an empty or null value imported cleanly and left behind a server that libpq would silently authenticate as the OS account running pgAdmin rather than reject outright. Check the value, matching the truthiness check already used for shared servers and the "Username must be specified" rule enforced by the server dialog. --- web/pgadmin/utils/__init__.py | 8 +++++--- web/pgadmin/utils/tests/test_validate_json_data.py | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/web/pgadmin/utils/__init__.py b/web/pgadmin/utils/__init__.py index d459e72b99d..0a57d7e6c0f 100644 --- a/web/pgadmin/utils/__init__.py +++ b/web/pgadmin/utils/__init__.py @@ -649,9 +649,11 @@ def check_is_integer(value): "found for server '%s'" % server ) else: - errmsg = check_attrib("Username") - if errmsg: - return errmsg + if not obj.get("Username"): + return gettext( + "'Username' attribute not found for server '%s'" % + server + ) errmsg = check_attrib("MaintenanceDB") if errmsg: diff --git a/web/pgadmin/utils/tests/test_validate_json_data.py b/web/pgadmin/utils/tests/test_validate_json_data.py index b5525d8eed3..4360f9dccb0 100644 --- a/web/pgadmin/utils/tests/test_validate_json_data.py +++ b/web/pgadmin/utils/tests/test_validate_json_data.py @@ -47,6 +47,20 @@ class TestValidateJsonData(BaseTestGenerator): expected_error="'Username' attribute not found", expected_servers=["1"] )), + ('A non-shared server with an empty username is rejected', + dict( + servers={"1": server(Username="")}, + is_admin=True, + expected_error="'Username' attribute not found", + expected_servers=["1"] + )), + ('A non-shared server with a null username is rejected', + dict( + servers={"1": server(Username=None)}, + is_admin=True, + expected_error="'Username' attribute not found", + expected_servers=["1"] + )), ('A shared server with only a shared username is valid', dict( servers={"1": server(Shared=True, SharedUsername="postgres")}, From be8985e8d0498a3ed0a149840ebeb6b6f80a3d2d Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 12:39:43 +0100 Subject: [PATCH 5/6] fix: run BEGIN/COMMIT/ROLLBACK on a plain cursor under server cursor mode (#8991) execute_void() blindly reused whatever cursor was cached for the connection, which under "server cursor" mode is the named/server-side AsyncDictServerCursor left over from the last SELECT. A named cursor's execute() always wraps the statement as `DECLARE ... CURSOR FOR `, which cannot express a transaction-control statement, so BEGIN/COMMIT/ROLLBACK silently failed (failing one step earlier still, on a `prepare` keyword the server-side cursor's execute() doesn't accept at all) and the exception was swallowed by the background query thread. The transaction was therefore never actually committed or rolled back, and the next poll() picked up the previous query's leftover column info, which is what made the result grid appear instead of the Messages tab. Run the statement through a throwaway plain cursor instead, leaving the cached server-side cursor untouched, and clear the stale column info so poll() correctly reports no result set. --- .../utils/driver/psycopg3/connection.py | 13 +++ .../tests/test_execute_void_server_cursor.py | 85 +++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py diff --git a/web/pgadmin/utils/driver/psycopg3/connection.py b/web/pgadmin/utils/driver/psycopg3/connection.py index d07a16cefcd..d8a6cd53172 100644 --- a/web/pgadmin/utils/driver/psycopg3/connection.py +++ b/web/pgadmin/utils/driver/psycopg3/connection.py @@ -1173,6 +1173,19 @@ def execute_void(self, query, params=None, formatted_exception_msg=False): if not status: return False, str(cur) + + if isinstance(cur, AsyncDictServerCursor): + # A named/server-side cursor's execute() always runs the query + # as `DECLARE ... CURSOR FOR `, which cannot express a + # transaction-control statement such as BEGIN/COMMIT/ROLLBACK. + # Run this one statement through a throwaway plain cursor + # instead, leaving the cached server-side cursor untouched, and + # treat it as leaving no result set for whatever poll() call + # comes next. + cur = self.conn.cursor() + self.column_info = None + self.row_count = 0 + query_id = str(secrets.choice(range(1, 9999999))) current_app.logger.log( diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py new file mode 100644 index 00000000000..c885f66df8e --- /dev/null +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py @@ -0,0 +1,85 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Regression test: ``execute_void()`` must not run a transaction-control +statement (BEGIN/COMMIT/ROLLBACK) through a cached named/server-side +cursor. + +A named cursor's ``execute()`` always wraps the statement as +``DECLARE ... CURSOR FOR ``, which cannot express BEGIN/COMMIT/ +ROLLBACK. Before the fix, the Commit/Rollback buttons under "server +cursor" mode silently did nothing: the DECLARE-wrapped call failed +(actually failing one step earlier, on a ``prepare`` keyword the +server-side cursor's ``execute()`` doesn't accept at all), the exception +was swallowed by the background query thread, and the next poll() then +reported the *previous* query's leftover column info, making the result +grid appear instead of the Messages tab (pgAdmin issue #8991).""" + +from unittest.mock import MagicMock, patch + +from pgadmin.utils.driver.psycopg3.connection import Connection +from pgadmin.utils.driver.psycopg3.cursor import AsyncDictServerCursor +from pgadmin.utils.route import BaseTestGenerator + + +class ExecuteVoidServerCursorTest(BaseTestGenerator): + + scenarios = [ + ('COMMIT with a cached server-side cursor runs on a throwaway ' + 'plain cursor and clears stale column info', dict(sql='COMMIT;')), + ('ROLLBACK with a cached server-side cursor runs on a throwaway ' + 'plain cursor and clears stale column info', + dict(sql='ROLLBACK;')), + ] + + def runTest(self): + manager = MagicMock(sid=1) + conn = Connection(manager, 'test-conn-id', 'testdb') + conn.python_encoding = 'utf-8' + + # Leftover state from a previous SELECT executed through the + # server-side cursor. + conn.column_info = [{'name': 'x'}] + conn.row_count = 1 + + server_cursor = MagicMock(spec=AsyncDictServerCursor) + server_cursor.closed = False + + plain_cursor = MagicMock() + plain_cursor.closed = False + + conn.conn = MagicMock() + conn.conn.cursor.return_value = plain_cursor + conn.conn.info.user = 'postgres' + conn.conn.info.host = 'localhost' + conn.conn.info.dbname = 'testdb' + + # current_user needs a real request context to resolve at all; + # patch it only once inside that context, to a stand-in with the + # attribute execute_void()'s log line reads. + with self.app.test_request_context(): + with patch( + 'pgadmin.utils.driver.psycopg3.connection.current_user', + MagicMock(email='test@example.com') + ), patch.object(Connection, '_Connection__cursor', + return_value=(True, server_cursor)): + status, result = conn.execute_void(self.sql) + + self.assertTrue(status) + self.assertIsNone(result) + + # The statement ran on the throwaway plain cursor, not the + # cached server-side one. + plain_cursor.execute.assert_called_once() + server_cursor.execute.assert_not_called() + + # Stale result-set state from the prior SELECT must not leak + # into whatever poll() call comes next. + self.assertIsNone(conn.column_info) + self.assertEqual(conn.row_count, 0) From 767c8b91a123afa6611341dd1c3aefc384de53ca Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 13:04:11 +0100 Subject: [PATCH 6/6] fix: exclude non-editable alias/expression columns from UPDATE saves in Query Tool (#10103) --- .../sqleditor/utils/save_changed_data.py | 15 ++++ .../utils/tests/test_save_changed_data.py | 84 +++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/web/pgadmin/tools/sqleditor/utils/save_changed_data.py b/web/pgadmin/tools/sqleditor/utils/save_changed_data.py index 78776697ff3..c9c72316d8a 100644 --- a/web/pgadmin/tools/sqleditor/utils/save_changed_data.py +++ b/web/pgadmin/tools/sqleditor/utils/save_changed_data.py @@ -177,6 +177,21 @@ def save_changed_data(changed_data, columns_info, conn, command_obj, list_of_sql[of_type] = [] for each_row in changed_data[of_type]: data = changed_data[of_type][each_row]['data'] + + # Drop any column the client included that isn't a real + # editable column of the underlying table (e.g. + # `first_name || ' ' || last_name as the_name`). The + # frontend already marks such columns as non-editable + # (shown with a lock icon), but still includes them in + # the changed data. Without this guard the rendered + # UPDATE references a non-existent column and Postgres + # rejects it. Issue #10103. + data = { + k: v for k, v in data.items() + if k in columns_info and + columns_info[k].get('is_editable', True) + } + pk_escaped = { pk: pk_val.replace('%', '%%') if hasattr( pk_val, 'replace') else pk_val diff --git a/web/pgadmin/tools/sqleditor/utils/tests/test_save_changed_data.py b/web/pgadmin/tools/sqleditor/utils/tests/test_save_changed_data.py index b43d0773a9a..6d5c79c0c2c 100644 --- a/web/pgadmin/tools/sqleditor/utils/tests/test_save_changed_data.py +++ b/web/pgadmin/tools/sqleditor/utils/tests/test_save_changed_data.py @@ -1040,3 +1040,87 @@ def _create_test_table(self): "FROM {0};" ).format(self.test_table_name) utils.create_table_with_query(self.server, self.db_name, create_sql) + + +class TestSaveUpdatedRowSkipsNonEditableColumn(TestSaveChangedData): + """Regression test for issue #10103. + + When a Query Tool result includes an expression or alias column + (e.g. ``first_name || ' ' || last_name AS the_name``), editing a + real column on an existing row must not include the alias in the + generated UPDATE statement. The alias is not a real column of the + underlying table, so pgAdmin already flags it as non-editable (shown + with a lock icon in the grid) but the update save flow used to send + it anyway, causing PostgreSQL to reject the statement with + ``column "the_name" does not exist``. + """ + + scenarios = [ + ('Update a real column while an aliased expression column is ' + 'present', dict( + save_payload={ + "updated": { + "1": { + "err": False, + "data": { + "first_name": "Jane", + # The client includes the aliased expression + # column in the changed data even though it + # is marked non-editable. Sending it must + # not break the UPDATE. + "the_name": "Jane Doe" + }, + "primary_keys": {"id": 1} + } + }, + "added": {}, + "staged_rows": {}, + "deleted": {}, + "updated_index": {"1": "1"}, + "added_index": {}, + "columns": [ + {"name": "id", "pos": 0, "can_edit": True, + "type": "integer", "cell": "number", + "not_null": True, "has_default_val": False, + "is_array": False, "display_name": "id"}, + {"name": "first_name", "pos": 1, "can_edit": True, + "type": "text", "cell": "string", + "not_null": False, "has_default_val": False, + "is_array": False, "display_name": "first_name"}, + {"name": "last_name", "pos": 2, "can_edit": True, + "type": "text", "cell": "string", + "not_null": False, "has_default_val": False, + "is_array": False, "display_name": "last_name"}, + {"name": "the_name", "pos": 3, "can_edit": False, + "type": "text", "cell": "string", + "not_null": False, "has_default_val": False, + "is_array": False, "display_name": "the_name"}, + ] + }, + save_status=True, + check_sql='SELECT id, first_name, last_name ' + 'FROM %s WHERE id = 1', + check_result=[[1, "Jane", "Doe"]] + )), + ] + + def _create_test_table(self): + self.test_table_name = "test_for_save_data_alias_" + \ + str(secrets.choice(range(1000, 9999))) + create_sql = """ + DROP TABLE IF EXISTS "{0}"; + + CREATE TABLE "{0}"( + id INT PRIMARY KEY, + first_name TEXT, + last_name TEXT + ); + + INSERT INTO "{0}" VALUES (1, 'John', 'Doe'); + """.format(self.test_table_name) + self.select_sql = ( + "SELECT id, first_name, last_name, " + "first_name || ' ' || last_name AS the_name " + "FROM {0};" + ).format(self.test_table_name) + utils.create_table_with_query(self.server, self.db_name, create_sql)