Allow ADMIN OPTION holders to manage Group Role membership. #9450 (#10315)

Users holding ADMIN OPTION on a role, directly or through an inherited
membership as PostgreSQL itself allows, can now change its members,
whilst all other property changes still require CREATEROLE.
This commit is contained in:
Dave Page committed 2026-10-12 23:23:48 +01:00
1 parent 0cbddf21ba
commit e85db0fb8b
7 files changed
+521 -9

No files matched your search

@@ -567,6 +567,18 @@ rolmembership:{
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
# 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),
@@ -619,6 +631,7 @@ rolmembership:{
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 +640,15 @@ rolmembership:{
(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,9 +680,25 @@ rolmembership:{
self.role = row['rolname']
self.rolCanLogin = row['rolcanlogin']
self.rolSuper = row['rolsuper']
self.has_admin_option = row.get('has_admin_option', False)
return False, ''
def _has_admin_option(self, rid):
"""Return True if the connecting user may GRANT/REVOKE membership
in the role 'rid', as determined by permission.sql. Any failure to
look it up counts as False, so the dialog fails closed."""
status, res = self.conn.execute_dict(
render_template(
self.sql_path + 'permission.sql',
rid=rid,
conn=self.conn
)
)
if not status or len(res['rows']) == 0:
return False
return bool(res['rows'][0].get('has_admin_option', False))
def check_precondition(action=None):
"""
This function will behave as a decorator which will checks the status
@@ -713,16 +751,20 @@ rolmembership:{
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
@@ -895,6 +937,11 @@ rolmembership:{
res['rows'][0]['is_sys_obj'] = (
res['rows'][0]['oid'] <= self._DATABASE_LAST_SYSTEM_OID or
self.datistemplate)
# Tell the dialog whether the connecting user may manage this role's
# membership (ADMIN OPTION, held directly or through another role).
# The same permission.sql drives the update guard, so the UI and the
# backend cannot disagree about who qualifies.
res['rows'][0]['has_admin_option'] = self._has_admin_option(rid)
res = {**res['rows'][0], 'variables': variables['rows']}
return ajax_response(
response=res,
@@ -1023,6 +1070,19 @@ rolmembership:{
@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.
# request_keys is set by validate_request; if that has not run the
# guard must fail closed rather than let the request through.
if getattr(self, 'membership_only_update', False) and \
not getattr(self, 'request_keys', set()) <= \
{'rolmembers', 'oid'}:
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,
@@ -41,7 +41,6 @@ export default class RoleSchema extends BaseUISchema {
this.isReadOnly = null;
this.nodeInfo = this.fieldOptions.nodeInfo;
this.user = this.nodeInfo.server.user;
}
get idAttribute() {
@@ -55,6 +54,19 @@ 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, directly or
// through another role. The backend works that out (has_admin_option in
// the properties response) with the same query that gates the update,
// so it is not re-derived from the member list here.
isMemberAdmin(state) {
return Boolean(state?.has_admin_option);
}
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.'),
},
@@ -0,0 +1,32 @@
SELECT
rolname, rolcanlogin, rolsuper,
-- Whether the connecting user may GRANT/REVOKE membership in this role.
-- On PostgreSQL 16 and later the server's own check
-- (check_role_membership_authorization and select_best_admin in user.c)
-- is has_admin_privs_of_role(): ADMIN OPTION held directly, or through a
-- chain of memberships each granted WITH INHERIT TRUE. pg_has_role(...,
-- 'WITH ADMIN OPTION') still uses is_admin_of_role(), which ignores
-- INHERIT and so says yes where the server answers "no possible
-- grantors", hence the explicit walk over pg_auth_members here.
(
SELECT rolsuper FROM pg_catalog.pg_roles WHERE rolname = current_user
) OR EXISTS (
WITH RECURSIVE admin_path(roleid, admin_option, inherit_option) AS (
SELECT am.roleid, am.admin_option, am.inherit_option
FROM pg_catalog.pg_auth_members am
WHERE am.member = (
SELECT oid FROM pg_catalog.pg_roles
WHERE rolname = current_user
)
UNION
SELECT am.roleid, am.admin_option, am.inherit_option
FROM pg_catalog.pg_auth_members am
JOIN admin_path p ON am.member = p.roleid
WHERE p.inherit_option
)
SELECT 1 FROM admin_path
WHERE roleid = {{ rid }}::OID AND admin_option
) AS has_admin_option
FROM
pg_catalog.pg_roles
WHERE oid = {{ rid }}::OID
@@ -1,5 +1,14 @@
SELECT
rolname, rolcanlogin, rolsuper
rolname, rolcanlogin, rolsuper,
-- Whether the connecting user may GRANT/REVOKE membership in this role.
-- On PostgreSQL 14 and 15 the server's own check (AddRoleMems/DelRoleMems
-- in user.c) is is_admin_of_role(), which walks pg_auth_members without
-- regard to rolinherit, and pg_has_role(..., 'WITH ADMIN OPTION') calls
-- exactly that function, so this matches the server's rule, including
-- ADMIN OPTION held through another role the current user is a member of.
pg_catalog.pg_has_role(
current_user, {{ rid }}::OID, 'USAGE WITH ADMIN OPTION'
) AS has_admin_option
FROM
pg_catalog.pg_roles
WHERE oid = {{ rid }}::OID
@@ -0,0 +1,144 @@
##########################################################################
#
# pgAdmin 4 - PostgreSQL Tools
#
# Copyright (C) 2013 - 2026, The pgAdmin Development Team
# This software is released under the PostgreSQL Licence
#
##########################################################################
import uuid
from flask import render_template
from pgadmin.utils.route import BaseTestGenerator
from regression import parent_node_dict
from regression.python_test_utils import test_utils as utils
class RoleAdminOptionPermissionTestCase(BaseTestGenerator):
"""Live test of the has_admin_option column in permission.sql.
The column must say exactly what the server itself allows for
GRANT/REVOKE of the role: ADMIN OPTION held directly, or through
another role the current user is a member of. On PostgreSQL 16 and
later a membership granted WITH INHERIT FALSE does not carry the
ADMIN OPTION along, whereas on 14 and 15 it does. Each case is
checked both against the expected value and against whether a real
GRANT as that role succeeds, so the template cannot drift from the
server's rule unnoticed.
"""
scenarios = [
('Check Role Node', dict(url='/browser/role/obj/'))
]
def setUp(self):
self.server_id = parent_node_dict["server"][-1]["server_id"]
suffix = str(uuid.uuid4())[1:8]
self.target = 'adm_target_%s' % suffix
self.newmember = 'adm_newmember_%s' % suffix
self.holder = 'adm_holder_%s' % suffix
self.direct = 'adm_direct_%s' % suffix
self.inherited = 'adm_inherited_%s' % suffix
self.noinherit = 'adm_noinherit_%s' % suffix
self.plain = 'adm_plain_%s' % suffix
self.member = 'adm_member_%s' % suffix
self.all_roles = [
self.target, self.newmember, self.holder, self.direct,
self.inherited, self.noinherit, self.plain, self.member
]
self.connection = utils.get_db_connection(
self.server['db'], self.server['username'],
self.server['db_password'], self.server['host'],
self.server['port'], self.server['sslmode'])
self.connection.autocommit = True
self.server_version = self.connection.info.server_version
cur = self.connection.cursor()
for role in self.all_roles:
cur.execute('CREATE ROLE "%s"' % role)
# direct: ADMIN OPTION on the target itself.
cur.execute('GRANT "%s" TO "%s" WITH ADMIN OPTION' %
(self.target, self.direct))
# holder has ADMIN OPTION; inherited and noinherit are members of
# holder, with and without INHERIT respectively.
cur.execute('GRANT "%s" TO "%s" WITH ADMIN OPTION' %
(self.target, self.holder))
cur.execute('GRANT "%s" TO "%s"' % (self.holder, self.inherited))
if self.server_version >= 160000:
cur.execute('GRANT "%s" TO "%s" WITH INHERIT FALSE' %
(self.holder, self.noinherit))
else:
cur.execute('ALTER ROLE "%s" NOINHERIT' % self.noinherit)
cur.execute('GRANT "%s" TO "%s"' % (self.holder, self.noinherit))
# member: plain membership of the target, no ADMIN OPTION.
cur.execute('GRANT "%s" TO "%s"' % (self.target, self.member))
cur.execute("SELECT oid FROM pg_catalog.pg_roles WHERE rolname = %s",
(self.target,))
self.target_oid = cur.fetchone()[0]
cur.close()
def _has_admin_option(self, as_role):
with self.app.app_context():
sql = render_template(
'roles/sql/#{0}#/permission.sql'.format(self.server_version),
rid=self.target_oid, conn=None)
cur = self.connection.cursor()
try:
cur.execute('SET ROLE "%s"' % as_role)
cur.execute(sql)
row = cur.fetchone()
return row[3]
finally:
cur.execute('RESET ROLE')
cur.close()
def _server_allows_grant(self, as_role):
cur = self.connection.cursor()
try:
cur.execute('SET ROLE "%s"' % as_role)
try:
cur.execute('GRANT "%s" TO "%s"' %
(self.target, self.newmember))
except Exception:
return False
cur.execute('REVOKE "%s" FROM "%s"' %
(self.target, self.newmember))
return True
finally:
cur.execute('RESET ROLE')
cur.close()
def runTest(self):
noinherit_expected = self.server_version < 160000
cases = [
(self.direct, True),
(self.inherited, True),
(self.noinherit, noinherit_expected),
(self.plain, False),
(self.member, False),
]
for role, expected in cases:
self.assertEqual(
self._has_admin_option(role), expected,
'has_admin_option for %s' % role)
self.assertEqual(
self._server_allows_grant(role), expected,
'server GRANT authorisation for %s' % role)
# The properties response carries the flag for the dialog; the
# test server user is a superuser, who always qualifies.
response = self.tester.get(
self.url + str(utils.SERVER_GROUP) + '/' +
str(self.server_id) + '/' + str(self.target_oid),
follow_redirects=True)
self.assertEqual(response.status_code, 200)
self.assertTrue(response.json['has_admin_option'])
def tearDown(self):
cur = self.connection.cursor()
for role in self.all_roles:
cur.execute('DROP ROLE IF EXISTS "%s"' % role)
cur.close()
self.connection.close()
@@ -0,0 +1,218 @@
##########################################################################
#
# pgAdmin 4 - PostgreSQL Tools
#
# Copyright (C) 2013 - 2026, The pgAdmin Development Team
# This software is released under the PostgreSQL Licence
#
##########################################################################
import json
from unittest.mock import MagicMock, patch
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
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
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 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,
'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)
# 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)
# 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
@@ -45,5 +45,42 @@ describe('RoleSchema', ()=>{
it('properties', async ()=>{
await getPropertiesView(createSchemaObject(), getInitData);
});
describe('membersReadOnly', ()=>{
it('is read only for a plain user without ADMIN OPTION on the role', ()=>{
const schemaObj = createSchemaObject();
const state = {oid: 123, has_admin_option: false, rolmembers: [{role: 'postgres', admin: false}]};
expect(schemaObj.membersReadOnly(state)).toBe(true);
});
it('is read only when the backend has not said whether ADMIN OPTION is held', ()=>{
const schemaObj = createSchemaObject();
// Fail closed: a direct admin row in the member list is not enough on
// its own, the backend flag is the only thing consulted.
const state = {oid: 123, rolmembers: [{role: 'postgres', admin: true}]};
expect(schemaObj.membersReadOnly(state)).toBe(true);
});
it('is editable for a user the backend reports as holding ADMIN OPTION', ()=>{
const schemaObj = createSchemaObject();
// Covers ADMIN OPTION inherited through another role too: the user
// need not appear in the member list at all.
const state = {oid: 123, has_admin_option: true, rolmembers: []};
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);
});
});
});