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) }} diff --git a/web/pgadmin/browser/server_groups/servers/roles/__init__.py b/web/pgadmin/browser/server_groups/servers/roles/__init__.py index a2f407cda68..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), @@ -619,6 +626,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 +635,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 +675,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 +731,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 +1045,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 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 " + "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..f33bcfa3a3d --- /dev/null +++ b/web/pgadmin/browser/server_groups/servers/roles/tests/test_role_check_permission_unit_test.py @@ -0,0 +1,116 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +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 + + +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 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 ' 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); + }); + }); });