From cd04416bff73f2cb8ef9275cd3e225160445ab72 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 11:47:30 +0100 Subject: [PATCH 1/5] 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 0d2e316b10f65260900699033ba6fd626110388e Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 09:11:40 +0100 Subject: [PATCH 2/5] fix: validate membership-only update against pre-mutation request keys _validate_rolemembers() mutates the request dict in place, adding derived keys such as rol_members_list and rol_members_revoked_list. The membership-only update guard in RoleView.update() checked those mutated keys against {'rolmembers'}, so a valid ADMIN OPTION request containing only rolmembers was wrongly rejected as forbidden. Capture the client-supplied keys before validate_request() runs the validators, and check against that snapshot instead. Adds a regression test for the rolmembers-only update path. --- .../server_groups/servers/roles/__init__.py | 9 +++- .../test_role_check_permission_unit_test.py | 54 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/web/pgadmin/browser/server_groups/servers/roles/__init__.py b/web/pgadmin/browser/server_groups/servers/roles/__init__.py index 63a21d7887e..842c206054e 100644 --- a/web/pgadmin/browser/server_groups/servers/roles/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/roles/__init__.py @@ -567,6 +567,13 @@ def wrap(self, **kwargs): except ValueError: data[k] = v + # Capture the client-supplied keys before the validators below + # mutate 'data' (e.g. _validate_rolemembers adds derived keys + # such as 'rol_members_list'), so callers that need to know what + # the client actually sent (e.g. the membership-only update + # check) can rely on this instead of the mutated dict. + self.request_keys = set(data) + invalid_msg_arr = [ self._validate_rolname(kwargs.get('rid', -1), data), self._validate_rolvaliduntil(data), @@ -1039,7 +1046,7 @@ def create(self, gid, sid): @validate_request def update(self, gid, sid, rid): if getattr(self, 'membership_only_update', False) and \ - not set(self.request) <= {'rolmembers'}: + not self.request_keys <= {'rolmembers'}: return forbidden( _("The current user does not have permission to update " "the role. Users with ADMIN OPTION on this role may " 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 index 4319ec9ff4d..f33bcfa3a3d 100644 --- 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 @@ -60,3 +60,57 @@ def runTest(self): def tearDown(self): pass + + +class RoleMembersOnlyUpdateRequestKeysTest(BaseTestGenerator): + """Regression test for the membership-only update guard. + + _validate_rolemembers() mutates the request dict in place, adding + derived keys ('rol_members_list', 'rol_members_revoked_list') that + the client never sent. The membership-only update guard in + RoleView.update() must check the client-supplied keys captured + before that mutation (self.request_keys), not the mutated dict, + otherwise a valid ADMIN OPTION request containing only 'rolmembers' + would be wrongly rejected as forbidden. + """ + scenarios = [ + ('Check Role Node', dict(url='/browser/role/obj/')) + ] + + def setUp(self): + pass + + def runTest(self): + view = RoleView(cmd=None) + view.manager = MagicMock() + view.manager.version = 170000 + + data = { + 'rolmembers': { + 'added': [ + {'role': 'member_role', 'admin': True, + 'inherit': True, 'set': True} + ], + 'changed': [], + 'deleted': [] + } + } + + # Mirror what validate_request() does: capture the client + # supplied keys before running the validators. + request_keys = set(data) + + # This mutates 'data' in place, adding derived keys. + self.assertIsNone(view._validate_rolemembers(10, data)) + self.assertIn('rol_members_list', data) + + # The mutated dict is no longer a subset of {'rolmembers'} ... + self.assertFalse(set(data) <= {'rolmembers'}) + + # ... but the keys captured before mutation still are, so the + # membership-only guard (which must use request_keys) allows + # the request through instead of returning 403. + self.assertTrue(request_keys <= {'rolmembers'}) + + def tearDown(self): + pass From cf31b82e3681bd70ae8034ec628868713ebd3075 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Tue, 25 Aug 2026 09:56:37 +0100 Subject: [PATCH 3/5] test: exercise RoleView.update() for the ADMIN OPTION membership-only guard The existing regression test for the membership-only update guard re-implemented _check_permission()/_validate_rolemembers() logic by hand instead of calling validate_request() or RoleView.update(), so it wouldn't catch a regression in how those decorators interact. Add a test that drives RoleView.update() through its real decorator chain (check_precondition -> validate_request -> update), with the driver/connection/SQL rendering mocked out, submitting a rolmembers-only body as an ADMIN OPTION holder and asserting the request is not rejected with 403. --- .../test_role_check_permission_unit_test.py | 84 ++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) 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 index f33bcfa3a3d..bd74e0ff79c 100644 --- 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 @@ -7,7 +7,8 @@ # ########################################################################## -from unittest.mock import MagicMock +import json +from unittest.mock import MagicMock, patch from pgadmin.utils.route import BaseTestGenerator from pgadmin.browser.server_groups.servers.roles import RoleView @@ -114,3 +115,84 @@ def runTest(self): def tearDown(self): pass + + +class RoleUpdateAdminOptionMembershipOnlyTest(BaseTestGenerator): + """End-to-end regression test for the ADMIN OPTION membership-only + update guard. + + The two tests above exercise _check_permission() and + _validate_rolemembers() individually, but neither actually calls + validate_request() or RoleView.update(), so a regression that broke + how those two decorators interact (e.g. the membership-only guard + reading the wrong dict, or request_keys being set/consumed at the + wrong point in the chain) would slip past them. + + This test drives RoleView.update() through its real decorator chain + (check_precondition -> validate_request -> update) with the driver, + connection and SQL rendering mocked out, submitting a 'rolmembers' + -only body as a user who holds ADMIN OPTION on the target role (but + is neither a superuser nor a CREATEROLE holder), and asserts the + request is NOT rejected with 403. + """ + scenarios = [ + ('Check Role Node', dict(url='/browser/role/obj/')) + ] + + def setUp(self): + pass + + @patch('pgadmin.browser.server_groups.servers.roles.get_driver') + @patch('pgadmin.browser.server_groups.servers.roles.render_template') + def runTest(self, render_template_mock, get_driver_mock): + view = RoleView(cmd=None) + + manager = MagicMock() + manager.version = 170000 + manager.db_info = None + manager.user_info = { + 'is_superuser': False, 'can_create_role': False, 'id': 5 + } + + conn = MagicMock() + conn.connected.return_value = True + # Used for the permission lookup, the ALTER ROLE, and the + # post-update node fetch alike; has_admin_option=True is what + # drives the ADMIN OPTION carve-out in _check_permission(). + conn.execute_dict.return_value = (True, {'rows': [{ + 'rolname': 'grp_role', 'rolcanlogin': False, 'rolsuper': False, + 'has_admin_option': True, 'description': None + }]}) + manager.connection.return_value = conn + + get_driver_mock.return_value.connection_manager.return_value = \ + manager + + # The client sends only 'rolmembers' - exactly what an ADMIN + # OPTION holder (who may manage membership only) is allowed to + # change. + body = { + 'rolmembers': { + 'added': [ + {'role': 'member_role', 'admin': True, + 'inherit': True, 'set': True} + ], + 'changed': [], + 'deleted': [] + } + } + + with self.app.test_request_context( + data=json.dumps(body), content_type='application/json' + ): + response = view.update(gid=1, sid=1, rid=10) + + # The real _check_permission() call, driven off the mocked + # has_admin_option row, must have flagged this as a + # membership-only update ... + self.assertTrue(view.membership_only_update) + # ... and update() must let it through rather than forbidding it. + self.assertNotEqual(response.status_code, 403) + + def tearDown(self): + pass From f78a989414fba73f3607f43cbc0b302fb1c6b592 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 14:28:38 +0100 Subject: [PATCH 4/5] fix: allow the dialog's oid in ADMIN OPTION membership-only updates The properties dialog sends the role's 'oid' with every edit, because SchemaState.changes() appends the schema's idAttribute to the payload. The membership-only guard only allowed {'rolmembers'}, so a real save from the dialog by an ADMIN OPTION holder was still rejected with 403; the end-to-end test missed it because it built a body without 'oid'. Allow 'oid' as well (the role is identified by 'rid' from the URL, not by that value), send the dialog's real payload shape in the test, and assert that adding any other attribute is still forbidden. --- .../server_groups/servers/roles/__init__.py | 5 ++++- .../test_role_check_permission_unit_test.py | 18 +++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/roles/__init__.py b/web/pgadmin/browser/server_groups/servers/roles/__init__.py index 842c206054e..4a176d79950 100644 --- a/web/pgadmin/browser/server_groups/servers/roles/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/roles/__init__.py @@ -1045,8 +1045,11 @@ def create(self, gid, sid): @check_precondition(action='update') @validate_request def update(self, gid, sid, rid): + # The properties dialog always sends the role's 'oid' alongside the + # changed fields, so it is allowed here as well; the role being + # updated is identified by 'rid' from the URL, not by that value. if getattr(self, 'membership_only_update', False) and \ - not self.request_keys <= {'rolmembers'}: + not self.request_keys <= {'rolmembers', 'oid'}: return forbidden( _("The current user does not have permission to update " "the role. Users with ADMIN OPTION on this role may " 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 index bd74e0ff79c..c89469b1ad7 100644 --- 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 @@ -168,10 +168,13 @@ def runTest(self, render_template_mock, get_driver_mock): get_driver_mock.return_value.connection_manager.return_value = \ manager - # The client sends only 'rolmembers' - exactly what an ADMIN - # OPTION holder (who may manage membership only) is allowed to - # change. + # The body the properties dialog actually sends: the changed + # 'rolmembers' collection plus the role's 'oid', which + # SchemaState.changes() appends to every edit-mode payload. + # 'rolmembers' is exactly what an ADMIN OPTION holder (who may + # manage membership only) is allowed to change. body = { + 'oid': 10, 'rolmembers': { 'added': [ {'role': 'member_role', 'admin': True, @@ -194,5 +197,14 @@ def runTest(self, render_template_mock, get_driver_mock): # ... and update() must let it through rather than forbidding it. self.assertNotEqual(response.status_code, 403) + # Anything beyond membership is still forbidden to an ADMIN + # OPTION holder, so this can't be used to escalate the role. + body['rolsuper'] = True + with self.app.test_request_context( + data=json.dumps(body), content_type='application/json' + ): + response = view.update(gid=1, sid=1, rid=10) + self.assertEqual(response.status_code, 403) + def tearDown(self): pass From 781a79e159d7f4aabe3f7f59a1d35af841c9722f Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 23 Sep 2026 15:10:17 +0100 Subject: [PATCH 5/5] fix: reject a non-object role request body with 428 instead of a 500 validate_request() now takes set(data) to record the client-supplied keys, which raises TypeError for a JSON array body such as [{}]. Return a precondition error when the body is not a JSON object, and cover it in the update test. --- .../browser/server_groups/servers/roles/__init__.py | 5 +++++ .../roles/tests/test_role_check_permission_unit_test.py | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/web/pgadmin/browser/server_groups/servers/roles/__init__.py b/web/pgadmin/browser/server_groups/servers/roles/__init__.py index 4a176d79950..669eb9458eb 100644 --- a/web/pgadmin/browser/server_groups/servers/roles/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/roles/__init__.py @@ -567,6 +567,11 @@ def wrap(self, **kwargs): except ValueError: data[k] = v + if not isinstance(data, dict): + return precondition_required( + _("Request data must be a JSON object.") + ) + # Capture the client-supplied keys before the validators below # mutate 'data' (e.g. _validate_rolemembers adds derived keys # such as 'rol_members_list'), so callers that need to know what 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 index c89469b1ad7..01b61775ca2 100644 --- 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 @@ -206,5 +206,13 @@ def runTest(self, render_template_mock, get_driver_mock): response = view.update(gid=1, sid=1, rid=10) self.assertEqual(response.status_code, 403) + # A body that isn't a JSON object is rejected as a client error + # rather than failing with a server error. + with self.app.test_request_context( + data=json.dumps([{}]), content_type='application/json' + ): + response = view.update(gid=1, sid=1, rid=10) + self.assertEqual(response.status_code, 428) + def tearDown(self): pass