-
Notifications
You must be signed in to change notification settings - Fork 879
Restore tags and passfile for standard users on shared servers #10330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
bd252ca
1f8a075
4173ddf
0713356
be8985e
1edd7bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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.") | ||
| ) | ||
|
Comment on lines
+1041
to
+1047
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Check the original request fields before membership normalization.
Record the client-supplied key set before Proposed fix def wrap(self, **kwargs):
...
+ request_fields = set(data)
invalid_msg_arr = [
...
]
...
self.request = data
+ self.request_fields = request_fields
def update(self, gid, sid, rid):
if getattr(self, 'membership_only_update', False) and \
- not set(self.request) <= {'rolmembers'}:
+ not self.request_fields <= {'rolmembers'}:🤖 Prompt for AI Agents |
||
|
|
||
| sql = render_template( | ||
| self.sql_path + self._UPDATE_SQL, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 <query>`, 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 | ||
|
Comment on lines
+1177
to
+1187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Invalidate the cached async cursor for the temporary-cursor path.
Define the post-transaction 🤖 Prompt for AI Agents |
||
|
|
||
| query_id = str(secrets.choice(range(1, 9999999))) | ||
|
|
||
| current_app.logger.log( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: pgadmin-org/pgadmin4
Length of output: 2977
🌐 Web query:
site:github.com/pgadmin-org/pgadmin4 "def get_shared_server" "create_shared_server"💡 Result:
The functions get_shared_server and create_shared_server are internal methods associated with the pgAdmin 4 project, specifically related to its shared server management and data isolation architecture [1][2]. In the pgAdmin 4 codebase (as observed in developments regarding server mode and security hardening), these functions are utilized to manage shared server properties and ensure proper access control [1][2]: - get_shared_server: This method is used within the pgAdmin server modules (e.g., pgadmin.browser.server_groups.servers) to retrieve the shared server instance associated with a specific user and server ID, facilitating the resolution of shared server overrides [1]. It is also employed to simplify operations like clearing saved passwords or SSH tunnel passwords for shared servers [1][2]. - create_shared_server: This function handles the creation of shared server records [2]. It is responsible for initializing shared server instances, including copying specific configuration attributes such as tunnel ports and keep-alive settings from the owner of the shared server [1][2]. These methods were part of a broader effort to harden pgAdmin 4 against privilege escalation, credential leaks, and data integrity issues by enforcing strict isolation between server owners and other users [2].
Citations:
🌐 Web query:
site:github.com/pgadmin-org/pgadmin4 "passfile" "SharedServer" "data.tags"💡 Result:
The string "passfile" within the context of
SharedServeranddata.tagsrefers to internal configuration attributes managed by pgAdmin [1][2]. Specifically,SharedServeris a model used in pgAdmin to handle server registrations shared across users [1]. Recent architectural updates to pgAdmin have introduced dedicated columns to theSharedServermodel, includingtags, to ensure feature parity between shared and individual server registrations [1][2]. Regarding the specific terms: - SharedServer data.tags: In the pgAdmin codebase,tagsare part of the metadata persisted forSharedServerobjects [1][2]. These tags allow users to organize shared servers, and recent changes ensure that non-owner users can interact with these tags (such as reading or updating them) while maintaining correct differential application of changes [1][2]. - passfile: This is a connection parameter used to point pgAdmin to apgpassfile containing database credentials [3]. In a server registration (includingSharedServerdefinitions), you can specify apassfileparameter within theConnectionParametersJSON block [3][4][5]. This allows the application to authenticate automatically without prompting the user for a password [3]. When configuringpassfilefor shared servers in pgAdmin's "Server mode" (typically containerized environments), users must ensure the path is accessible to the pgAdmin process [6]. Because pgAdmin in server mode often restricts access to the file system, common practices include mapping apgpassfile into a specific storage location (often inside/var/lib/pgadmin/storage/or a relative path from the user's home directory) and referencing that path in theservers.jsonconfiguration [4][7][5]. Thepassfileparameter is prioritized by pgAdmin's connection manager to resolve credentials [8].Citations:
🌐 Web query:
"get_shared_server()" "create_shared_server()" pgadmin4 __init__.py💡 Result:
In the pgAdmin 4 codebase, specifically within web/pgadmin/browser/server_groups/servers/init.py, the methods get_shared_server and create_shared_server are static methods of the ServerModule class used to manage shared server records in server mode [1][2]. get_shared_server(server, gid) This method retrieves the SharedServer record for the current user. If a record does not exist for the current user and the specified server (identified by osid), it lazily creates one by calling create_shared_server [1]. It uses a query on the SharedServer model filtered by the current user's ID and the server's ID to ensure data isolation, and it handles concurrent requests using database transaction rollbacks if a unique constraint violation occurs [1][3]. create_shared_server(data, gid) This method is responsible for creating a new SharedServer record. It creates a persisted entry in the database that associates a server configuration with a specific user [1]. This mechanism allows pgAdmin to manage user-specific overrides (such as connection parameters or SSH tunnel settings) for shared servers while maintaining the integrity of the original server definition [3][2]. These methods are central to pgAdmin's server-mode architecture, which enables administrators to share server definitions across multiple users while ensuring that sensitive information, such as passwords or SSH tunnel configurations, remains isolated or correctly scoped to the individual user [3][2].
Citations:
🌐 Web query:
repo:pgadmin-org/pgadmin4 "SharedServer" "server_default" "tags"💡 Result:
In the pgAdmin 4 codebase, the SharedServer model includes a tags field designed to support feature parity with the standard Server model [1][2]. This allows non-owner users to maintain per-user values (such as tags) via an overlay mechanism, rather than inheriting or having their values suppressed by the owner's configuration [1][2]. Recent updates to pgAdmin 4 have refactored the SharedServer model and its interaction with the server overlay logic [1][2]. Specifically: 1. Persistence and Parity: The SharedServer schema was updated to include persisted columns—including tags, passexec_cmd, passexec_expiration, kerberos_conn, and post_connection_sql—to ensure that shared servers behave consistently for non-owners [1][2]. 2. Overlay Logic: The system handles tag operations by reading from and writing to the SharedServer record for non-owners [1][2]. When a non-owner updates a shared server, the application computes tag differences based on the shared server record (acting as the base) to ensure updates are applied correctly without overwriting existing tags or incorrectly inheriting the owner's tags [1][2]. 3. Default Configuration: In the migration and model definitions, fields like kerberos_conn are initialized using SQLAlchemy's portable boolean literals (e.g., server_default=sa.false) to ensure consistent behavior across different database backends [1][2]. Shared server functionality, including tag management, is a feature of pgAdmin 4's server mode, which allows administrators to share server definitions with other users [3]. Non-owner users see these servers in their browser interface and can interact with them according to defined permissions [3].
Citations:
🌐 Web query:
repo:pgadmin-org/pgadmin4 "sharedserver_feature_parity" migration💡 Result:
The sharedserver_feature_parity migration in pgAdmin 4 refers to a database schema update introduced to enhance the consistency and security of the Shared Server feature, particularly in server (multi-user) mode [1]. Key aspects of this migration include: Purpose and Functionality: The migration was designed to achieve feature parity for Shared Servers by ensuring that non-owner users have independent control over specific server parameters [1]. Previously, certain settings might have been inherited from the server owner; the migration allows non-owner users to maintain their own values for fields such as passexec_cmd, passexec_expiration, kerberos_conn, tags, and post_connection_sql [1]. It also enforces security by implementing write guards, preventing non-owners from modifying specific sensitive fields while allowing them to configure others (like post_connection_sql) that run under their own credentials [1]. Database Impact: This migration bumped the schema version (to 51) and added several columns to the SharedServer model while removing obsolete columns (such as db_res) [1]. It was part of a broader effort to harden data isolation, particularly in server mode, to address vulnerabilities related to cross-user data access [2][3]. Upgrade Considerations: Users upgrading to versions containing this migration (such as v9.15) have experienced issues during the migration process, including potential duplication of server groups and challenges with credentials [4]. The complexity of these changes means that users should ensure they have backups of their configuration database (pgadmin4.db) before performing upgrades, as downgrades often require restoring from a backup file (e.g., pgadmin4.db.prev.bak) due to the non-backward-compatible nature of these schema changes [5][4]. For general management of server configurations, including migration between instances, pgAdmin provides built-in import/export functionality via the setup.py CLI tool, which allows users to export server definitions to JSON and import them into other installations [6][7].
Citations:
🌐 Web query:
https://github.com/pgadmin-org/pgadmin4/pull/9830/files "passfile" "tags"💡 Result:
In pgAdmin 4, the "passfile" parameter is used to specify the file path to a PostgreSQL password file (.pgpass), which allows for authentication without manually providing a password [1][2][3][4]. This functionality relies on the libpq library [4]. Regarding the specific context of Pull Request
#9830and recent updates: 1. Recent Security Enhancements: Pull Request#9830(merged April 2026) focused on hardening server-mode deployments, specifically addressing data isolation, credential leaks, and privilege escalation vulnerabilities [5]. It enforced stricter access checks on various server operations to prevent unauthorized access in shared server environments [5]. 2. Precedence and Behavior: As of pgAdmin 4 version 9.16, the handling of credentials has been updated to prioritize the passfile [6]. If a server connection is configured with both an external password-exec command and a passfile, pgAdmin now defaults to using the passfile and ignores the password-exec command, logging a warning when this conflict occurs [6]. 3. Configuration and Usage: - In server import/export JSON files, the parameter is typically represented as "passfile" within the "ConnectionParameters" object [2]. - When running in pgAdmin's server mode, especially in containerized environments (like Docker), paths for the passfile can sometimes be interpreted relative to the user's storage directory unless specifically configured otherwise [7]. - Users should ensure the.pgpass file adheres to PostgreSQL security requirements, typically requiring permissions set to 0600 (read/write by owner only) to be accepted by libpq [3][7]. top_results: [5][2][6][3][4][7]Citations:
🌐 Web query:
https://github.com/pgadmin-org/pgadmin4/commit/e4edcf225311ebe5bb10985ef10f726117c93e4c "SharedServer" migration💡 Result:
The GitHub commit e4edcf2, associated with pull request
#9835, implements a migration for the SharedServer feature in pgAdmin 4 to improve feature parity and security [1][2]. Key details of this migration and the associated changes include: Feature Parity and Schema Updates: The migration adds five columns to the SharedServer model: passexec_cmd, passexec_expiration, kerberos_conn, tags, and post_connection_sql [1][2]. These additions allow non-owner users to maintain their own per-user values for these fields rather than inheriting them from the owner [1][2]. Additionally, the migration drops the db_res column, which was identified as unused by non-owners [1][2]. Security and Write Guards: To prevent unauthorized modification of sensitive configurations, the commit introduces an _owner_only_fields guard [1]. This mechanism blocks non-owners from setting sensitive fields (specifically passexec_cmd, passexec_expiration, post_connection_sql, db_res, and db_res_type) via the API [1][2]. Deployment Warning: Because this migration includes destructive schema changes (dropping a column), it has been associated with deployment hazards in containerized environments (such as Kubernetes) [3]. Specifically, if an environment uses a RollingUpdate deployment strategy where an older container version and a newer container version share a data volume, the newer version may execute the migration and drop the column while the older version still expects it to exist, causing runtime errors (e.g., sqlite3.OperationalError) [3]. Official guidance recommends using a Recreate deployment strategy instead of RollingUpdate and ensuring init containers do not run a newer version of the image than the main application container [3].Citations:
🌐 Web query:
site:github.com/pgadmin-org/pgadmin4/blob/master/web/pgadmin/browser/server_groups/servers/__init__.py "if shared_server"💡 Result:
The matching pgAdmin4 file is:
pgadmin4/web/pgadmin/browser/server_groups/servers/init.py
Relevant logic handles shared servers, including hiding them based on preferences and retrieving shared-server properties for non-owners. (github.com)
Citations:
Backfill existing
SharedServerrows.get_shared_server()only callscreate_shared_server()when no user-specific row exists. Existing rows therefore do not receive the copiedpassfileortags. Add a separate upgrade backfill or a missing-field-only refresh, preserve explicit per-user overrides, and add a regression test.🤖 Prompt for AI Agents