Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified docs/en_US/images/role_membership.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified docs/en_US/images/role_sql.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
21 changes: 16 additions & 5 deletions docs/en_US/role_dialog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,26 @@ Use the *Privileges* tab to grant privileges to the role.
* Move the *Bypass RLS?* switch to the *Yes* position to control whether a
role can bypasses every row-level security (RLS) policy. The default value is *No*.

Click the *Membership* tab to continue.

.. image:: images/role_membership.png
:alt: Role dialog membership tab
:align: center

* Specify member of the role in the *Member of* field and specify the members in the *Member* field.
Confirm each selection by checking the checkbox to the right of the role name;
delete a selection by clicking the *x* to the left of the role name.
Membership conveys the privileges granted to the specified role to each of
its members.
Use the *Membership* tab to define role memberships. A role can be a member of
other roles and can have other roles as members.

* Use *Member of* section to specify roles of which the current role
is a member. To assign *Admin Option* for a selected role, click on
the appropriate checkbox.
* Use *Members* section to specify roles that are members of the current
role. To assign *Admin Option* for a selected role, click on the appropriate checkbox.

Click the *Add* icon (+) to add more roles; to discard a
role, click the trash icon to the left of the row and confirm the deletion
in the *Delete Row* popup.

**Note:** Apart from *Admin Option*, *Inherit Option* and *Set Option* are available for both *Member of* section and *Members* section from PostgreSQL version >= 16.

Click the *Parameters* tab to continue.

Expand Down
204 changes: 124 additions & 80 deletions web/pgadmin/browser/server_groups/servers/roles/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,80 +207,65 @@ def _process_rolemembership(self, id, data):
:param id: id of role
:param data: input role data
"""
def _part_dict_list(dict_list, condition, list_key=None):
def _part_dict_list(dict_list, condition):
ret_val = []
for d in dict_list:
if condition(d):
ret_val.append(d[list_key])
ret_val.append(d)

return ret_val

if id == -1:
data['members'] = []
data['admins'] = []

data['admins'] = _part_dict_list(
data['rolmembership'], lambda d: d['admin'], 'role')
data['members'] = _part_dict_list(
data['rolmembership'], lambda d: not d['admin'], 'role')
data['rolmembership_list'] = data['rolmembership']
else:
data['admins'] = _part_dict_list(
data['rolmembership'].get('added', []),
lambda d: d['admin'], 'role')
data['members'] = _part_dict_list(
data['rolmembership'].get('added', []),
lambda d: not d['admin'], 'role')

data['admins'].extend(_part_dict_list(
data['rolmembership'].get('changed', []),
lambda d: d['admin'], 'role'))
data['revoked_admins'] = _part_dict_list(
data['rolmembership'].get('changed', []),
lambda d: not d['admin'], 'role')

data['revoked'] = _part_dict_list(
data['rolmembership'].get('deleted', []),
lambda _: True, 'role')
data['rolmembership_list'] = data['rolmembership'].get('added', [])

if self.manager.version < 160000:
data['rolmembership_list'].extend(_part_dict_list(
data['rolmembership'].get('changed', []),
lambda d: d['admin']))
data['rolmembership_revoked_admins'] = _part_dict_list(
data['rolmembership'].get('changed', []),
lambda d: not d['admin'])
else:
data['rolmembership_list'].extend(
data['rolmembership'].get('changed', []))

data['rolmembership_revoked_list'] = (
data['rolmembership'].get('deleted', []))

def _process_rolmembers(self, id, data):
"""
Parser role members.
:param id:
:param data:
"""
def _part_dict_list(dict_list, condition, list_key=None):
def _part_dict_list(dict_list, condition):
ret_val = []
for d in dict_list:
if condition(d):
ret_val.append(d[list_key])
ret_val.append(d)

return ret_val
if id == -1:
data['rol_members'] = []
data['rol_admins'] = []
data['rol_members_list'] = data['rolmembers']

data['rol_admins'] = _part_dict_list(
data['rolmembers'], lambda d: d['admin'], 'role')
data['rol_members'] = _part_dict_list(
data['rolmembers'], lambda d: not d['admin'], 'role')
else:
data['rol_admins'] = _part_dict_list(
data['rolmembers'].get('added', []),
lambda d: d['admin'], 'role')
data['rol_members'] = _part_dict_list(
data['rolmembers'].get('added', []),
lambda d: not d['admin'], 'role')

data['rol_admins'].extend(_part_dict_list(
data['rolmembers'].get('changed', []),
lambda d: d['admin'], 'role'))
data['rol_revoked_admins'] = _part_dict_list(
data['rolmembers'].get('changed', []),
lambda d: not d['admin'], 'role')

data['rol_revoked'] = _part_dict_list(
data['rolmembers'].get('deleted', []),
lambda _: True, 'role')
data['rol_members_list'] = data['rolmembers'].get('added', [])

if self.manager.version < 160000:
data['rol_members_list'].extend(_part_dict_list(
data['rolmembers'].get('changed', []),
lambda d: d['admin']))
data['rol_members_revoked_admins'] = _part_dict_list(
data['rolmembers'].get('changed', []),
lambda d: not d['admin'])
else:
data['rol_members_list'].extend(
data['rolmembers'].get('changed', []))

data['rol_members_revoked_list'] = (
data['rolmembers'].get('deleted', []))

Comment on lines 237 to 269
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n --hidden --no-ignore-vcs -S \
  "rolmembership_list|rolmembership_revoked_list|rolmembership_revoked_admins|rol_members_list|rol_members_revoked_list|rol_members_revoked_admins" .

Repository: pgadmin-org/pgadmin4

Length of output: 5901


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '200,280p'

Repository: pgadmin-org/pgadmin4

Length of output: 3434


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '1,150p'

Repository: pgadmin-org/pgadmin4

Length of output: 5592


🏁 Script executed:

rg -n "_process_rolmembers|_process_rolemembership" web/pgadmin/browser/server_groups/servers/roles/__init__.py | head -20

Repository: pgadmin-org/pgadmin4

Length of output: 369


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '295,360p'

Repository: pgadmin-org/pgadmin4

Length of output: 2446


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '440,520p'

Repository: pgadmin-org/pgadmin4

Length of output: 2473


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '520,600p'

Repository: pgadmin-org/pgadmin4

Length of output: 3165


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '600,700p'

Repository: pgadmin-org/pgadmin4

Length of output: 4366


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '700,800p'

Repository: pgadmin-org/pgadmin4

Length of output: 4112


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '800,900p'

Repository: pgadmin-org/pgadmin4

Length of output: 4440


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '900,1000p'

Repository: pgadmin-org/pgadmin4

Length of output: 3851


🏁 Script executed:

cat -n web/pgadmin/browser/server_groups/servers/roles/__init__.py | sed -n '1000,1100p'

Repository: pgadmin-org/pgadmin4

Length of output: 4006


🏁 Script executed:

cat web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sql | sed -n '35,65p'

Repository: pgadmin-org/pgadmin4

Length of output: 1340


🏁 Script executed:

cat web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sql | sed -n '50,130p'

Repository: pgadmin-org/pgadmin4

Length of output: 3035


Consider defensive copy in list assignments, but naming is intentional

The aliasing concern at lines 251 and 254 is valid—assigning data['rolmembers'].get('added', []) directly creates a reference that gets mutated via .extend() at lines 257 and 264. A defensive copy with list() is reasonable for safety.

However, the naming inconsistency claim is inaccurate. The two patterns (rolmembership_* vs rol_members_*) are intentional and required by templates: they correspond to the input dictionary keys (data['rolmembership'] vs data['rolmembers']). Template usage confirms both patterns are correctly wired throughout create.sql and update.sql.

Proposed adjustment (defensive copy)
         if id == -1:
-            data['rol_members_list'] = data['rolmembers']
+            data['rol_members_list'] = list(data['rolmembers'])
@@
         else:
-            data['rol_members_list'] = data['rolmembers'].get('added', [])
+            data['rol_members_list'] = list(data['rolmembers'].get('added', []))

Note: The same aliasing pattern exists in _process_rolemembership (lines 219, 221–232) and warrants the same treatment.

🤖 Prompt for AI Agents
In `@web/pgadmin/browser/server_groups/servers/roles/__init__.py` around lines 237
- 269, The code assigns lists from data['rolmembers'] directly (e.g., in
_process_rolmembers setting data['rol_members_list'] =
data['rolmembers'].get('added', []) and similar for changed/deleted) which
creates aliases that are later mutated with .extend(); make defensive copies
instead by wrapping those .get(...) results with list(...) so mutations do not
modify the original input structures, and apply the same defensive-copy change
in the analogous function _process_rolemembership for its list assignments.

def _validate_rolemembers(self, id, data):
"""
Expand All @@ -298,13 +283,21 @@ def _validate_rolemembers(self, id, data):

rolmembers:[{
role: [rolename],
admin: True/False
admin: True/False,
inherit: True/False,
set: True/False,
},
...
]""")

if not self._validate_input_dict_for_new(data['rolmembers'],
['role', 'admin']):
if (self.manager.version < 160000 and
not self._validate_input_dict_for_new(
data['rolmembers'], ['role', 'admin'])):
return msg
elif (self.manager.version >= 160000 and
not self._validate_input_dict_for_new(
data['rolmembers'],
['role', 'admin', 'inherit', 'set'])):
return msg

self._process_rolmembers(id, data)
Expand All @@ -316,26 +309,38 @@ def _validate_rolemembers(self, id, data):
rolmembers:{
'added': [{
role: [rolename],
admin: True/False
admin: True/False,
inherit: True/False,
set: True/False,
},
...
],
'deleted': [{
role: [rolename],
admin: True/False
admin: True/False,
inherit: True/False,
set: True/False,
},
...
],
'updated': [{
role: [rolename],
admin: True/False
admin: True/False,
inherit: True/False,
set: True/False,
},
...
]
""")
if not self._validate_input_dict_for_update(data['rolmembers'],
['role', 'admin'],
['role']):
if (self.manager.version < 160000 and
not self._validate_input_dict_for_update(
data['rolmembers'], ['role', 'admin'], ['role'])):
return msg
elif (self.manager.version >= 160000 and
not self._validate_input_dict_for_update(
data['rolmembers'],
['role', 'admin', 'inherit', 'set'],
['role'])):
return msg

self._process_rolmembers(id, data)
Expand All @@ -357,13 +362,21 @@ def _validate_rolemembership(self, id, data):

rolmembership:[{
role: [rolename],
admin: True/False
admin: True/False,
inherit: True/False,
set: True/False,
},
...
]""")

if not self._validate_input_dict_for_new(
data['rolmembership'], ['role', 'admin']):
if (self.manager.version < 160000 and
not self._validate_input_dict_for_new(
data['rolmembership'], ['role', 'admin'])):
return msg
elif (self.manager.version >= 160000 and
not self._validate_input_dict_for_new(
data['rolmembership'],
['role', 'admin', 'inherit', 'set'])):
return msg

self._process_rolemembership(id, data)
Expand All @@ -375,25 +388,38 @@ def _validate_rolemembership(self, id, data):
rolmembership:{
'added': [{
role: [rolename],
admin: True/False
admin: True/False,
inherit: True/False,
set: True/False,
},
...
],
'deleted': [{
role: [rolename],
admin: True/False
admin: True/False,
inherit: True/False,
set: True/False,
},
...
],
'updated': [{
role: [rolename],
admin: True/False
admin: True/False,
inherit: True/False,
set: True/False,
},
...
]
""")
if not self._validate_input_dict_for_update(
data['rolmembership'], ['role', 'admin'], ['role']):
if (self.manager.version < 160000 and
not self._validate_input_dict_for_update(
data['rolmembership'], ['role', 'admin'], ['role'])):
return msg
elif (self.manager.version >= 160000 and
not self._validate_input_dict_for_update(
data['rolmembership'],
['role', 'admin', 'inherit', 'set'],
['role'])):
return msg

self._process_rolemembership(id, data)
Expand Down Expand Up @@ -792,16 +818,25 @@ def _set_seclabels(self, row):
})
row['seclabels'] = res

def _set_rolemembership(self, row):
def _set_rolemembers(self, row):

if 'rolmembers' in row and row['rolmembers'] is not None:
rolmembers = []
for role in row['rolmembers']:
role = re.search(r'([01])(.+)', role)
rolmembers.append({
'role': role.group(2),
'admin': True if role.group(1) == '1' else False
})
if self.manager.version < 160000:
role = re.search(r'([01])(.+)', role)
rolmembers.append({
'role': role.group(2),
'admin': True if role.group(1) == '1' else False
})
else:
role = re.search(r'([01])([01])([01])(.+)', role)
rolmembers.append({
'role': role.group(4),
'admin': True if role.group(1) == '1' else False,
'inherit': True if role.group(2) == '1' else False,
'set': True if role.group(3) == '1' else False
})
row['rolmembers'] = rolmembers

Comment on lines +821 to 841
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Harden _set_rolemembers parsing: re.search(...) can be None → AttributeError
If row['rolmembers'] contains any unexpected string (or empty), re.search(...) can return None and group() will crash (Line 827-839). Given this runs in properties/list flows, it’s worth hardening.

Proposed hardening (use anchored match + guard)
     def _set_rolemembers(self, row):
@@
             for role in row['rolmembers']:
                 if self.manager.version < 160000:
-                    role = re.search(r'([01])(.+)', role)
+                    m = re.match(r'^([01])(.+)$', role or '')
+                    if not m:
+                        continue
                     rolmembers.append({
-                        'role': role.group(2),
-                        'admin': True if role.group(1) == '1' else False
+                        'role': m.group(2),
+                        'admin': m.group(1) == '1'
                     })
                 else:
-                    role = re.search(r'([01])([01])([01])(.+)', role)
+                    m = re.match(r'^([01])([01])([01])(.+)$', role or '')
+                    if not m:
+                        continue
                     rolmembers.append({
-                        'role': role.group(4),
-                        'admin': True if role.group(1) == '1' else False,
-                        'inherit': True if role.group(2) == '1' else False,
-                        'set': True if role.group(3) == '1' else False
+                        'role': m.group(4),
+                        'admin': m.group(1) == '1',
+                        'inherit': m.group(2) == '1',
+                        'set': m.group(3) == '1'
                     })

def transform(self, rset):
Expand All @@ -810,14 +845,23 @@ def transform(self, rset):
roles = row['rolmembership']
row['rolpassword'] = ''
for role in roles:
role = re.search(r'([01])(.+)', role)
res.append({
'role': role.group(2),
'admin': True if role.group(1) == '1' else False
})
if self.manager.version < 160000:
role = re.search(r'([01])(.+)', role)
res.append({
'role': role.group(2),
'admin': True if role.group(1) == '1' else False
})
else:
role = re.search(r'([01])([01])([01])(.+)', role)
res.append({
'role': role.group(4),
'admin': True if role.group(1) == '1' else False,
'inherit': True if role.group(2) == '1' else False,
'set': True if role.group(3) == '1' else False
})
Comment on lines +848 to +861
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Same regex None handling issue in transform() method.

The re.search() calls at lines 849 and 855 have the same vulnerability as _set_rolemembers - no guards against None results when the pattern doesn't match.

🐛 Proposed fix
             for role in roles:
                 if self.manager.version < 160000:
-                    role = re.search(r'([01])(.+)', role)
+                    m = re.match(r'^([01])(.+)$', role or '')
+                    if not m:
+                        continue
                     res.append({
-                        'role': role.group(2),
-                        'admin': True if role.group(1) == '1' else False
+                        'role': m.group(2),
+                        'admin': m.group(1) == '1'
                     })
                 else:
-                    role = re.search(r'([01])([01])([01])(.+)', role)
+                    m = re.match(r'^([01])([01])([01])(.+)$', role or '')
+                    if not m:
+                        continue
                     res.append({
-                        'role': role.group(4),
-                        'admin': True if role.group(1) == '1' else False,
-                        'inherit': True if role.group(2) == '1' else False,
-                        'set': True if role.group(3) == '1' else False
+                        'role': m.group(4),
+                        'admin': m.group(1) == '1',
+                        'inherit': m.group(2) == '1',
+                        'set': m.group(3) == '1'
                     })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self.manager.version < 160000:
role = re.search(r'([01])(.+)', role)
res.append({
'role': role.group(2),
'admin': True if role.group(1) == '1' else False
})
else:
role = re.search(r'([01])([01])([01])(.+)', role)
res.append({
'role': role.group(4),
'admin': True if role.group(1) == '1' else False,
'inherit': True if role.group(2) == '1' else False,
'set': True if role.group(3) == '1' else False
})
if self.manager.version < 160000:
m = re.match(r'^([01])(.+)$', role or '')
if not m:
continue
res.append({
'role': m.group(2),
'admin': m.group(1) == '1'
})
else:
m = re.match(r'^([01])([01])([01])(.+)$', role or '')
if not m:
continue
res.append({
'role': m.group(4),
'admin': m.group(1) == '1',
'inherit': m.group(2) == '1',
'set': m.group(3) == '1'
})
🤖 Prompt for AI Agents
In `@web/pgadmin/browser/server_groups/servers/roles/__init__.py` around lines 848
- 861, In transform(), the re.search() calls used to parse role strings can
return None and are not checked; update the transform() logic to verify the
regex match before accessing groups: after calling re.search(...) assign to a
variable (e.g., match), if match is None skip/handle the malformed entry (e.g.,
continue or log and skip) and only then use match.group(...) to build the dict
appended to res; apply this for both the pre-160000 and else branches that
reference match.group(1..4).

row['rolmembership'] = res
self._set_seclabels(row)
self._set_rolemembership(row)
self._set_rolemembers(row)

@check_precondition(action='properties')
def properties(self, gid, sid, rid):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,12 @@ export default class RoleSchema extends BaseUISchema {
memberDataFormatter(rawData) {
let members = '';
if(_.isObject(rawData)) {
const serverVersion = this.nodeInfo && this.nodeInfo.server && this.nodeInfo.server.version || 0;
rawData.forEach(member => {
let withAdmin = '';
if(member.admin) { withAdmin = ' [WITH ADMIN]';}
let badges = serverVersion >= 160000 ? ` [WITH ADMIN ${member.admin.toString().toUpperCase()}, INHERIT ${member.inherit.toString().toUpperCase()}, SET ${member.set.toString().toUpperCase()}]` : member.admin ? ' [WITH ADMIN OPTION]' : '';

if (members.length > 0) { members += ', '; }
members = members + (member.role + withAdmin);
members = members + (member.role + badges);
Comment on lines +61 to +66
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add defensive checks for potentially undefined member properties.

For PostgreSQL 16+, the code accesses member.inherit and member.set directly. If these properties are undefined or null (e.g., from legacy data or API inconsistencies), calling .toString() will throw a runtime error.

🔧 Suggested defensive fix
-      const serverVersion = this.nodeInfo && this.nodeInfo.server && this.nodeInfo.server.version || 0;
+      const serverVersion = this.nodeInfo?.server?.version || 0;
       rawData.forEach(member => {
-        let badges = serverVersion >= 160000 ? ` [WITH ADMIN ${member.admin.toString().toUpperCase()}, INHERIT ${member.inherit.toString().toUpperCase()}, SET ${member.set.toString().toUpperCase()}]` : member.admin ? ' [WITH ADMIN OPTION]' : '';
+        let badges = '';
+        if (serverVersion >= 160000) {
+          const admin = (member.admin ?? false).toString().toUpperCase();
+          const inherit = (member.inherit ?? false).toString().toUpperCase();
+          const set = (member.set ?? true).toString().toUpperCase();
+          badges = ` [WITH ADMIN ${admin}, INHERIT ${inherit}, SET ${set}]`;
+        } else {
+          badges = member.admin ? ' [WITH ADMIN OPTION]' : '';
+        }

         if (members.length > 0) { members += ', '; }
         members = members + (member.role + badges);
🤖 Prompt for AI Agents
In `@web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js` around
lines 61 - 66, The code building the badges inside rawData.forEach uses
member.admin, member.inherit and member.set directly (when computing badges for
serverVersion >= 160000) which can throw if those properties are undefined;
update the badge construction in the rawData.forEach callback (the block using
serverVersion, member.admin, member.inherit, member.set and appending to
members) to defensively coerce missing values to safe defaults (e.g., use
(member.inherit ?? false).toString().toUpperCase(), (member.set ??
false).toString().toUpperCase(), and similar for admin, and ensure member.role
is present before concatenation) so undefined/null won’t cause .toString() to
throw and concatenation into members is safe.

});
}
return members;
Expand Down Expand Up @@ -177,7 +177,7 @@ export default class RoleSchema extends BaseUISchema {
type: 'text',
controlProps: {
formatter: {
fromRaw: obj.memberDataFormatter,
fromRaw: (raw) => obj.memberDataFormatter(raw),
},
}
},
Expand All @@ -198,7 +198,7 @@ export default class RoleSchema extends BaseUISchema {
type: 'text',
controlProps: {
formatter: {
fromRaw: obj.memberDataFormatter,
fromRaw: (raw) => obj.memberDataFormatter(raw),
},
}
},
Expand Down
Loading
Loading