Open
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
WalkthroughCentralized server/group access-control added via new server_access utilities; models gain a UserScopedMixin and DebuggerFunctionArguments adds a user_id PK; migrations updated; many modules refactored to use access helpers and to scope queries/processes to the current user; tests adjusted and new isolation tests added. Changes
Sequence DiagramsequenceDiagram
participant Client as User Request
participant Endpoint as Endpoint Handler
participant Access as get_server / server_access
participant DB as SQLAlchemy / Models
participant Response as HTTP Response
Client->>Endpoint: request (e.g., GET /browser/server/obj/{sid})
Endpoint->>Access: get_server(sid, only_owned?)
Access->>DB: query Server/ServerGroup with access rules
DB-->>Access: Server object or None
alt Server returned
Access->>Endpoint: Server object
Endpoint->>Response: 200 OK + data
else None returned
Access->>Endpoint: None
Endpoint->>Response: 410 Gone + error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
asheshv
force-pushed
the
fix/data-isolation-bugs
branch
from
b913595 to
2e59998
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/pgadmin/tools/schema_diff/__init__.py (1)
373-390:⚠️ Potential issue | 🔴 CriticalCheck accessibility before touching the connection manager.
Lines 375-383 still probe
connection_manager(sid)andconn.connected()before the new guard on Line 385. That puts the security check behind a manager lookup/connection-state probe, so unauthorized requests can bypass the intended 410 path for already-connected servers or fail upstream before the access check runs.🔒 Suggested fix
def connect_server(sid): # Check if server is already connected then no need to reconnect again. - driver = get_driver(PG_DEFAULT_DRIVER) - manager = driver.connection_manager(sid) - conn = manager.connection() - if conn.connected(): - return make_json_response( - success=1, - info=gettext("Server connected."), - data={} - ) - server = get_accessible_server(sid) if server is None: return make_json_response( status=410, success=0, errormsg=gettext("Could not find the required server.") ) + driver = get_driver(PG_DEFAULT_DRIVER) + manager = driver.connection_manager(sid) + conn = manager.connection() + if conn.connected(): + return make_json_response( + success=1, + info=gettext("Server connected."), + data={} + ) view = SchemaDiffRegistry.get_node_view('server') return view.connect(server.servergroup_id, sid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/schema_diff/__init__.py` around lines 373 - 390, The connect_server function currently calls get_driver/get_connection_manager and inspects conn.connected() before verifying access; move the access check so get_accessible_server(sid) is called first and returns the 410 path if None before any calls to get_driver, manager, or conn. In practice, in connect_server, call get_accessible_server(sid) and handle the None case immediately, then proceed to get_driver(PG_DEFAULT_DRIVER), manager = driver.connection_manager(sid) and conn = manager.connection() and the existing connected() logic.web/pgadmin/browser/server_groups/servers/__init__.py (1)
2167-2174:⚠️ Potential issue | 🔴 CriticalClear the current user’s shared-server tunnel secret, not the owner’s.
For shared servers,
get_accessible_server(sid)still returns the baseServerrow. Clearingserver.tunnel_passwordhere wipes the owner’s saved SSH tunnel password for every user of that shared server.clear_saved_password()right above already has the correctSharedServerbranch; this path needs the same treatment.🔐 Suggested fix
def clear_sshtunnel_password(self, gid, sid): """ This function is used to remove sshtunnel password stored into the pgAdmin's db file. @@ :return: """ try: server = get_accessible_server(sid) + shared_server = None if server is None: return make_json_response( success=0, info=self.not_found_error_msg() ) - setattr(server, 'tunnel_password', None) + if server.shared and server.user_id != current_user.id: + shared_server = SharedServer.query.filter_by( + name=server.name, user_id=current_user.id, + servergroup_id=gid, osid=server.id + ).first() + if shared_server is None: + return make_json_response( + success=0, + info=gettext("Could not find the required server.") + ) + setattr(shared_server, 'tunnel_password', None) + else: + setattr(server, 'tunnel_password', None) db.session.commit()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/browser/server_groups/servers/__init__.py` around lines 2167 - 2174, The code is clearing the owner Server.tunnel_password for shared servers; instead, detect when the accessible server is a shared instance and clear the SharedServer row for the current user. Modify the branch after get_accessible_server(sid) to check for a SharedServer (or query SharedServer by server_id and current_user.id), set that SharedServer's tunnel_password = None and commit; otherwise keep the existing behavior of clearing Server.tunnel_password for non-shared servers. Use the same pattern/logic as clear_saved_password() and ensure db.session.commit() is called after updating the correct record.web/pgadmin/tools/sqleditor/__init__.py (1)
2412-2434:⚠️ Potential issue | 🟠 MajorApply per-user
SharedServeroverrides before buildingserver_list.This route now scopes which rows are returned, but it still serializes raw
Serverobjects. For shared servers, non-owners will get the owner-sidename,username, colors, andconnection_paramsin the new-connection dialog instead of theirSharedServervalues. That breaks the per-user customization model and can expose owner-specific connection metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/__init__.py` around lines 2412 - 2434, The loop currently serializes raw Server rows (from get_user_server_query) into server_group_data, which ignores per-user SharedServer overrides; fetch the SharedServer override for the current user for each Server (e.g. via SharedServer.query.filter_by(server_id=server.id, user_id=current_user.id) or an existing helper) and apply its override fields (name, username, fgcolor, bgcolor, connection_params, host/port/service as present) before calling server_icon_and_background and before building the dict; ensure convert_connection_parameter is called on the overridden connection_params and use the overridden values when populating 'label', 'value', 'fgcolor','bgcolor','host','port','service','connection_params' and when computing 'image' so the per-user customizations are reflected in server_group_data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/migrations/versions/add_user_id_to_debugger_func_args_.py`:
- Around line 102-118: The migration currently swallows all errors when running
op.execute over index_stmts, hiding real failures; change the error handling
around op.execute so only "table does not exist" (or equivalent message from the
DB) is ignored and any other exception is re-raised. Update the loop that
iterates index_stmts calling op.execute (refer to index_stmts and op.execute) to
catch the exception into a variable, inspect the error message/type (e.g.,
sqlalchemy.exc.* or check "does not exist" in str(e).lower()), continue only for
missing-table errors, and re-raise for any other error so the migration fails
loudly.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py`:
- Line 2321: MViewNode.check_utility_exists() is still doing an owner-only
server lookup while refresh_data() accepts shared servers; update the precheck
to use the same access helper by replacing the hardcoded owner-only lookup with
get_accessible_server(sid) (the same helper used in refresh_data()), ensuring
the check obtains the accessible server object and then proceeds with the
existing utility existence logic.
In `@web/pgadmin/browser/server_groups/servers/utils.py`:
- Around line 695-698: The sid branch of delete_adhoc_servers currently only
filters by Server.id (and optionally Server.user_id), risking deletion of
persisted servers; update the query built in delete_adhoc_servers so that when
filtering by sid you also add Server.is_adhoc == 1 to the query (i.e., refine q
= db.session.query(Server).filter(Server.id == sid) to include Server.is_adhoc
== 1, and preserve the existing optional filter on Server.user_id ==
current_user.id before calling q.delete()).
In `@web/pgadmin/tools/import_export/__init__.py`:
- Line 297: IEMessage.get_server_name() currently uses an owner-only server
lookup causing shared servers to appear as "Not available"; change its server
lookup to use the same access-aware helper (get_accessible_server(sid)) used
elsewhere so shared servers are returned. Locate the IEMessage.get_server_name
method and replace the owner-restricted query with a call to
get_accessible_server(sid) (or refactor to delegate to that helper) and ensure
any subsequent null/None handling remains consistent.
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 522-526: _init_sqleditor() returns a bare Response when
get_accessible_server(sid) is None, but callers (initialize_sqleditor,
update_sqleditor_connection, check_and_upgrade_to_qt) expect to unpack four
values; change the not-found branch to return the same 4-tuple shape (e.g.
return internal_server_error(...), None, None, None) so the callers can safely
unpack the response plus the other values; update the branch that uses
get_accessible_server(sid) to return the Response as the first element and
placeholders (None) for the remaining three.
---
Outside diff comments:
In `@web/pgadmin/browser/server_groups/servers/__init__.py`:
- Around line 2167-2174: The code is clearing the owner Server.tunnel_password
for shared servers; instead, detect when the accessible server is a shared
instance and clear the SharedServer row for the current user. Modify the branch
after get_accessible_server(sid) to check for a SharedServer (or query
SharedServer by server_id and current_user.id), set that SharedServer's
tunnel_password = None and commit; otherwise keep the existing behavior of
clearing Server.tunnel_password for non-shared servers. Use the same
pattern/logic as clear_saved_password() and ensure db.session.commit() is called
after updating the correct record.
In `@web/pgadmin/tools/schema_diff/__init__.py`:
- Around line 373-390: The connect_server function currently calls
get_driver/get_connection_manager and inspects conn.connected() before verifying
access; move the access check so get_accessible_server(sid) is called first and
returns the 410 path if None before any calls to get_driver, manager, or conn.
In practice, in connect_server, call get_accessible_server(sid) and handle the
None case immediately, then proceed to get_driver(PG_DEFAULT_DRIVER), manager =
driver.connection_manager(sid) and conn = manager.connection() and the existing
connected() logic.
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 2412-2434: The loop currently serializes raw Server rows (from
get_user_server_query) into server_group_data, which ignores per-user
SharedServer overrides; fetch the SharedServer override for the current user for
each Server (e.g. via SharedServer.query.filter_by(server_id=server.id,
user_id=current_user.id) or an existing helper) and apply its override fields
(name, username, fgcolor, bgcolor, connection_params, host/port/service as
present) before calling server_icon_and_background and before building the dict;
ensure convert_connection_parameter is called on the overridden
connection_params and use the overridden values when populating 'label',
'value', 'fgcolor','bgcolor','host','port','service','connection_params' and
when computing 'image' so the per-user customizations are reflected in
server_group_data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 92206711-07e0-4821-9336-efaba5d30128
📒 Files selected for processing (23)
web/migrations/versions/add_user_id_to_debugger_func_args_.pyweb/migrations/versions/ca00ec32581b_.pyweb/pgadmin/browser/server_groups/__init__.pyweb/pgadmin/browser/server_groups/servers/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.pyweb/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.pyweb/pgadmin/browser/server_groups/servers/utils.pyweb/pgadmin/browser/server_groups/tests/test_sg_data_isolation.pyweb/pgadmin/misc/bgprocess/processes.pyweb/pgadmin/misc/cloud/__init__.pyweb/pgadmin/misc/workspaces/__init__.pyweb/pgadmin/model/__init__.pyweb/pgadmin/tools/debugger/__init__.pyweb/pgadmin/tools/erd/__init__.pyweb/pgadmin/tools/import_export/__init__.pyweb/pgadmin/tools/psql/__init__.pyweb/pgadmin/tools/schema_diff/__init__.pyweb/pgadmin/tools/sqleditor/__init__.pyweb/pgadmin/tools/user_management/__init__.pyweb/pgadmin/utils/__init__.pyweb/pgadmin/utils/driver/psycopg3/__init__.pyweb/pgadmin/utils/server_access.py
web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py
Outdated
Show resolved
Hide resolved
asheshv
force-pushed
the
fix/data-isolation-bugs
branch
from
2e59998 to
b5b16d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
web/pgadmin/tools/user_management/__init__.py (1)
750-772:⚠️ Potential issue | 🟠 MajorDelete debugger argument rows before deleting the user.
The new
DebuggerFunctionArguments.user_idforeign key means this cleanup block now needs to remove those rows as well. Otherwise a user who has used the debugger can fail deletion on commit, or keep orphaned config state on backends without FK enforcement.🧹 Minimal fix
-from pgadmin.model import db, Role, User, UserPreference, Server, \ - ServerGroup, Process, Setting, roles_users, SharedServer +from pgadmin.model import db, Role, User, UserPreference, Server, \ + ServerGroup, Process, Setting, roles_users, SharedServer, \ + DebuggerFunctionArguments @@ Process.for_user(user_id=uid).delete() + DebuggerFunctionArguments.for_user(user_id=uid).delete() # Delete Shared servers for current user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/user_management/__init__.py` around lines 750 - 772, The cleanup block must also delete DebuggerFunctionArguments rows tied to the user to avoid FK errors or orphaned state; add a call like DebuggerFunctionArguments.query.filter_by(user_id=uid).delete(synchronize_session=False) (or equivalent) alongside the other deletes (e.g., near Process.for_user(user_id=uid).delete() / SharedServer deletes) before db.session.delete(usr) and db.session.commit(); reference the DebuggerFunctionArguments model and its user_id FK when placing the new delete.web/pgadmin/browser/server_groups/servers/databases/__init__.py (1)
553-609:⚠️ Potential issue | 🟠 MajorGuard
connection_manager()returningNone.After the new server-access check in
connection_manager(), these endpoints can now getNonefor an unauthorized or missing server. Both methods dereferencemanagerimmediately, which turns an access denial into a 500 instead of the handled not-found response used bycheck_precondition().🛡️ Suggested guard
def connect(self, gid, sid, did): """Connect the Database.""" from pgadmin.utils.driver import get_driver manager = get_driver(PG_DEFAULT_DRIVER).connection_manager(sid) + if manager is None: + return gone(errormsg=_("Could not find the server.")) conn = manager.connection(did=did, auto_reconnect=True) @@ def disconnect(self, gid, sid, did): """Disconnect the database.""" # Release Connection from pgadmin.utils.driver import get_driver manager = get_driver(PG_DEFAULT_DRIVER).connection_manager(sid) + if manager is None: + return gone(errormsg=_("Could not find the server.")) conn = manager.connection(did=did, auto_reconnect=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/browser/server_groups/servers/databases/__init__.py` around lines 553 - 609, Both connect() and disconnect() assume get_driver(...).connection_manager(sid) returns a manager and dereference it immediately; after the new server-access check connection_manager() may return None causing a 500. Add a guard after obtaining manager in both connect() and disconnect(): if manager is None, return the same handled response used by check_precondition() (i.e., call/check_precondition or the equivalent not-found/unauthorized response) instead of proceeding to use manager; reference the connection_manager call and the manager variable in connect() and disconnect() to locate where to add this early return.web/pgadmin/misc/workspaces/__init__.py (1)
164-178:⚠️ Potential issue | 🟠 MajorClone the current user’s shared-server view, not the base server row.
This branch can now clone shared servers, but
server.clone()still copies the ownerServerrecord. The adhoc server will lose per-userSharedServersettings such as name, service/SSH fields, saved passwords, and passfile/SSL overrides unless the shared overlay is applied before cloning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/misc/workspaces/__init__.py` around lines 164 - 178, The code currently calls get_accessible_server(...) then server.clone(), which clones the base Server row and loses per-user SharedServer overlays; instead fetch the user's SharedServer overlay for the accessible server (e.g., via the SharedServer model lookup for the current user and server id), merge/apply that SharedServer overlay onto the server object (copying fields like name, service/ssh, saved_passwords, passfile/ssl overrides) and only then call server.clone() (or clone from the composed object) so the adhoc server preserves per-user SharedServer settings; update the logic around get_accessible_server, server.clone(), and where connection_params/username/role/is_adhoc are set to operate on the overlay-merged server instance.web/pgadmin/tools/import_export/__init__.py (1)
99-110:⚠️ Potential issue | 🟠 MajorResolve the caller’s
SharedServerbefore formatting or launching the job.
get_accessible_server()only checks visibility. On shared servers, this code still reads the owner row forget_server_name(),env['PGUSER'], andp.set_env_variables(server, ...), so import/export jobs can run with the wrong username/passfile/SSL overrides and show the wrong server label. Materialize the current user’sSharedServeroverlay here before usingserver.Also applies to: 295-303, 389-403
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/import_export/__init__.py` around lines 99 - 110, get_accessible_server() only checks visibility but does not apply per-user SharedServer overlays, so get_server_name() and subsequent job setup (env['PGUSER'], p.set_env_variables(server, ...)) can use the owner/server row instead of the current user's overlay; resolve the caller’s SharedServer overlay before formatting or launching the job by replacing the raw server object returned from get_accessible_server(self.sid) with the per-user SharedServer-resolved server (use the SharedServer resolution helper or query to materialize the overlay) in get_server_name() and in the places that set env['PGUSER'] and call p.set_env_variables(server, ...) so that username, passfile and SSL overrides come from the caller’s SharedServer overlay.web/pgadmin/tools/sqleditor/__init__.py (1)
230-239:⚠️ Potential issue | 🟠 MajorShared-server overrides are still skipped in two SQL Editor paths.
initialize_viewdata()readsserver.password/name/usernamedirectly, andget_new_connection_data()serializesname, colors,service, andconnection_paramsfrom the baseServerrow. Shared users will therefore reconnect/prompt against owner-side credentials and see owner-side metadata in the new-connection dialog. Reuse the same shared-server resolution block that_init_sqleditor()andpanel()already use.Also applies to: 259-265, 2413-2435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/__init__.py` around lines 230 - 239, The code in initialize_viewdata() and get_new_connection_data() is directly reading Server.row fields (password/name/username) and must instead reuse the shared-server resolution logic used by _init_sqleditor() and panel(); locate the shared-server resolution block (the get_accessible_server(sid) + shared-owner override logic) and apply it where initialize_viewdata() and get_new_connection_data() pull server.password/name/username so the resolved Server (not the raw owner row) is used; update these functions to call the same resolution helper (or inline the same block) and then set kwargs['encpass'] (and any name/username values) from the resolved server object, repeating the change for the other occurrences noted (around the other ranges mentioned).
🧹 Nitpick comments (3)
web/pgadmin/tools/restore/tests/test_batch_process.py (1)
137-138: MockProcess.for_user()directly.Aliasing
for_usertoquery.filter_bylets a regression back toProcess.query.filter_by(...)keep passing, so this no longer proves the scoped API is being used on this security-sensitive path. The same pattern is repeated in the backup and maintenance batch-process tests.♻️ Suggested test setup
- process_mock.for_user = process_mock.query.filter_by - mock_result = process_mock.query.filter_by.return_value + mock_result = process_mock.for_user.return_value mock_result.first.return_value = TestMockProcess( restore_obj, self.class_params['args'], self.class_params['cmd']) @@ popen_mock.side_effect = popenMockSideEffect p.start() + process_mock.for_user.assert_called() self.assertTrue(popen_mock.called)- process_mock.for_user = process_mock.query.filter_by - process_mock.query.filter_by.return_value = [ + process_mock.for_user.return_value = [ TestMockProcess(restore_obj, self.class_params['args'], self.class_params['cmd']) ] @@ update_process_info_mock.return_value = [True, True] ret_value = p.list() + process_mock.for_user.assert_called() self.assertEqual(1, len(ret_value))Also applies to: 178-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/restore/tests/test_batch_process.py` around lines 137 - 138, The test currently aliases Process.for_user to Process.query.filter_by (via process_mock.for_user = process_mock.query.filter_by) which masks regressions; instead mock the scoped API directly by setting process_mock.for_user = mock.Mock(return_value=mock_result) or by patching Process.for_user to return mock_result so assertions exercise the for_user call (replace the aliasing at the process_mock.for_user assignment and the similar setup at lines 178-179); ensure any assertions verify process_mock.for_user(...) was called rather than query.filter_by.web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py (2)
57-58: Preferself.fail(...)over raising genericExceptionin tests.At Line 57-58 and Line 107-108, use unittest-native failure reporting for clearer test output.
Suggested fix
- if not self.server_id: - raise Exception("Server not found to test isolation") + if not self.server_id: + self.fail("Server not found to test isolation") ... - if not self.server_id: - raise Exception("Server not found to test shared access") + if not self.server_id: + self.fail("Server not found to test shared access")Also applies to: 107-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py` around lines 57 - 58, The test uses raise Exception("Server not found to test isolation") (and the similar raise at the other location) which should be replaced with the unittest TestCase failure helper to produce proper test output; update the test methods that check self.server_id (and the second identical check) to call self.fail("Server not found to test isolation") instead of raising Exception so the failure is reported by the test runner (look for uses of self.server_id in test_server_data_isolation tests).
31-34: Use immutablescenariosto avoid RUF012 mutable-class-attribute warnings.At Line 31-34 and Line 82-85,
scenarioscan be tuples since they are static definitions.Suggested fix
- scenarios = [ + scenarios = ( ('User B gets 410 for User A private server', dict(is_positive_test=False)), - ] + ) ... - scenarios = [ + scenarios = ( ('User B can access shared server from User A', dict(is_positive_test=True)), - ] + )Also applies to: 82-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py` around lines 31 - 34, Change the mutable list assigned to the variable scenarios into an immutable tuple in this test file: replace the list literal for scenarios (e.g., scenarios = [ ('User B gets 410 for User A private server', dict(is_positive_test=False)), ]) with a tuple literal (e.g., scenarios = ( ... ,)) at both occurrences (the one around lines 31-34 and the one around lines 82-85) so the static test definitions are immutable; keep the same tuple elements and structure, only change the enclosing brackets from [] to ().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/browser/server_groups/servers/__init__.py`:
- Around line 1820-1823: The credential endpoints currently call
get_accessible_server(sid) but still update the base Server row, which mutates
owners' credentials for shared servers; update change_password() and
clear_sshtunnel_password() to follow the same selection logic used in
clear_saved_password(): detect when the accessible server is a SharedServer
instance and perform updates against the caller's SharedServer record (not the
base Server), ensuring you load or create the SharedServer for the current user
and mutate its password fields (SharedServer.password /
SharedServer.ssh_tunnel_password) and save that object instead of Server;
reference the functions get_accessible_server(), change_password(),
clear_sshtunnel_password(), clear_saved_password(), and the SharedServer model
when making the change.
In
`@web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py`:
- Around line 45-52: The test currently dereferences
response_data['node']['_id'] immediately after posting (via self.tester.post)
which can raise a noisy exception if creation failed; before assigning
self.server_id, assert the HTTP response indicates success (check
response.status_code or response.status), assert response.data decodes to JSON
and that 'node' and '_id' keys exist (e.g. assert 'node' in response_data and
'_id' in response_data['node']), and only then set self.server_id =
response_data['node']['_id']; apply the same validation/checks to both
occurrences where response_data['node']['_id'] is used so failures produce clear
assertion messages instead of KeyError/TypeError.
In `@web/pgadmin/tools/erd/__init__.py`:
- Line 560: The call site guarding only panel rendering is insufficient; update
the backend path that dereferences connections so unauthorized requests don't
500 by adding a None check after connection_manager(sid) inside _get_connection
(and any callers like the /initialize, /prerequisite, /sql handlers and socket
handlers that use _get_connection). Specifically, detect when
connection_manager(sid) returns None, and return/raise an appropriate HTTP error
(e.g., 403 Forbidden or abort with a JSON error) instead of continuing to
dereference the object; ensure helpers like get_accessible_server remain
unchanged and that _get_connection consistently enforces the access boundary.
---
Outside diff comments:
In `@web/pgadmin/browser/server_groups/servers/databases/__init__.py`:
- Around line 553-609: Both connect() and disconnect() assume
get_driver(...).connection_manager(sid) returns a manager and dereference it
immediately; after the new server-access check connection_manager() may return
None causing a 500. Add a guard after obtaining manager in both connect() and
disconnect(): if manager is None, return the same handled response used by
check_precondition() (i.e., call/check_precondition or the equivalent
not-found/unauthorized response) instead of proceeding to use manager; reference
the connection_manager call and the manager variable in connect() and
disconnect() to locate where to add this early return.
In `@web/pgadmin/misc/workspaces/__init__.py`:
- Around line 164-178: The code currently calls get_accessible_server(...) then
server.clone(), which clones the base Server row and loses per-user SharedServer
overlays; instead fetch the user's SharedServer overlay for the accessible
server (e.g., via the SharedServer model lookup for the current user and server
id), merge/apply that SharedServer overlay onto the server object (copying
fields like name, service/ssh, saved_passwords, passfile/ssl overrides) and only
then call server.clone() (or clone from the composed object) so the adhoc server
preserves per-user SharedServer settings; update the logic around
get_accessible_server, server.clone(), and where
connection_params/username/role/is_adhoc are set to operate on the
overlay-merged server instance.
In `@web/pgadmin/tools/import_export/__init__.py`:
- Around line 99-110: get_accessible_server() only checks visibility but does
not apply per-user SharedServer overlays, so get_server_name() and subsequent
job setup (env['PGUSER'], p.set_env_variables(server, ...)) can use the
owner/server row instead of the current user's overlay; resolve the caller’s
SharedServer overlay before formatting or launching the job by replacing the raw
server object returned from get_accessible_server(self.sid) with the per-user
SharedServer-resolved server (use the SharedServer resolution helper or query to
materialize the overlay) in get_server_name() and in the places that set
env['PGUSER'] and call p.set_env_variables(server, ...) so that username,
passfile and SSL overrides come from the caller’s SharedServer overlay.
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 230-239: The code in initialize_viewdata() and
get_new_connection_data() is directly reading Server.row fields
(password/name/username) and must instead reuse the shared-server resolution
logic used by _init_sqleditor() and panel(); locate the shared-server resolution
block (the get_accessible_server(sid) + shared-owner override logic) and apply
it where initialize_viewdata() and get_new_connection_data() pull
server.password/name/username so the resolved Server (not the raw owner row) is
used; update these functions to call the same resolution helper (or inline the
same block) and then set kwargs['encpass'] (and any name/username values) from
the resolved server object, repeating the change for the other occurrences noted
(around the other ranges mentioned).
In `@web/pgadmin/tools/user_management/__init__.py`:
- Around line 750-772: The cleanup block must also delete
DebuggerFunctionArguments rows tied to the user to avoid FK errors or orphaned
state; add a call like
DebuggerFunctionArguments.query.filter_by(user_id=uid).delete(synchronize_session=False)
(or equivalent) alongside the other deletes (e.g., near
Process.for_user(user_id=uid).delete() / SharedServer deletes) before
db.session.delete(usr) and db.session.commit(); reference the
DebuggerFunctionArguments model and its user_id FK when placing the new delete.
---
Nitpick comments:
In
`@web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py`:
- Around line 57-58: The test uses raise Exception("Server not found to test
isolation") (and the similar raise at the other location) which should be
replaced with the unittest TestCase failure helper to produce proper test
output; update the test methods that check self.server_id (and the second
identical check) to call self.fail("Server not found to test isolation") instead
of raising Exception so the failure is reported by the test runner (look for
uses of self.server_id in test_server_data_isolation tests).
- Around line 31-34: Change the mutable list assigned to the variable scenarios
into an immutable tuple in this test file: replace the list literal for
scenarios (e.g., scenarios = [ ('User B gets 410 for User A private server',
dict(is_positive_test=False)), ]) with a tuple literal (e.g., scenarios = ( ...
,)) at both occurrences (the one around lines 31-34 and the one around lines
82-85) so the static test definitions are immutable; keep the same tuple
elements and structure, only change the enclosing brackets from [] to ().
In `@web/pgadmin/tools/restore/tests/test_batch_process.py`:
- Around line 137-138: The test currently aliases Process.for_user to
Process.query.filter_by (via process_mock.for_user =
process_mock.query.filter_by) which masks regressions; instead mock the scoped
API directly by setting process_mock.for_user =
mock.Mock(return_value=mock_result) or by patching Process.for_user to return
mock_result so assertions exercise the for_user call (replace the aliasing at
the process_mock.for_user assignment and the similar setup at lines 178-179);
ensure any assertions verify process_mock.for_user(...) was called rather than
query.filter_by.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbd03b26-818f-4e1b-8abd-d3ac1fc81307
📒 Files selected for processing (27)
web/migrations/versions/add_user_id_to_debugger_func_args_.pyweb/migrations/versions/ca00ec32581b_.pyweb/pgadmin/browser/server_groups/__init__.pyweb/pgadmin/browser/server_groups/servers/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.pyweb/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.pyweb/pgadmin/browser/server_groups/servers/utils.pyweb/pgadmin/browser/server_groups/tests/test_sg_data_isolation.pyweb/pgadmin/misc/bgprocess/processes.pyweb/pgadmin/misc/cloud/__init__.pyweb/pgadmin/misc/workspaces/__init__.pyweb/pgadmin/model/__init__.pyweb/pgadmin/tools/backup/tests/test_batch_process.pyweb/pgadmin/tools/debugger/__init__.pyweb/pgadmin/tools/erd/__init__.pyweb/pgadmin/tools/import_export/__init__.pyweb/pgadmin/tools/import_export/tests/test_batch_process.pyweb/pgadmin/tools/maintenance/tests/test_batch_process_maintenance.pyweb/pgadmin/tools/psql/__init__.pyweb/pgadmin/tools/restore/tests/test_batch_process.pyweb/pgadmin/tools/schema_diff/__init__.pyweb/pgadmin/tools/sqleditor/__init__.pyweb/pgadmin/tools/user_management/__init__.pyweb/pgadmin/utils/__init__.pyweb/pgadmin/utils/driver/psycopg3/__init__.pyweb/pgadmin/utils/server_access.py
🚧 Files skipped from review as they are similar to previous changes (10)
- web/pgadmin/misc/cloud/init.py
- web/pgadmin/tools/psql/init.py
- web/migrations/versions/ca00ec32581b_.py
- web/pgadmin/utils/init.py
- web/pgadmin/misc/bgprocess/processes.py
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/init.py
- web/pgadmin/tools/debugger/init.py
- web/migrations/versions/add_user_id_to_debugger_func_args_.py
- web/pgadmin/tools/schema_diff/init.py
- web/pgadmin/browser/server_groups/init.py
web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py
Show resolved
Hide resolved
web/pgadmin/tools/erd/__init__.py
Outdated
| is_linux_platform = True | ||
|
|
||
| s = Server.query.filter_by(id=int(params['sid'])).first() | ||
| s = get_accessible_server(int(params['sid'])) |
There was a problem hiding this comment.
This only protects the panel render.
get_accessible_server() here blocks the initial HTML load, but the direct ERD routes still flow through _get_connection(), which dereferences connection_manager(sid) without a None check. With the new access boundary in connection_manager(), unauthorized calls to /initialize, /prequisite, /sql, or socket tables will still 500.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/tools/erd/__init__.py` at line 560, The call site guarding only
panel rendering is insufficient; update the backend path that dereferences
connections so unauthorized requests don't 500 by adding a None check after
connection_manager(sid) inside _get_connection (and any callers like the
/initialize, /prerequisite, /sql handlers and socket handlers that use
_get_connection). Specifically, detect when connection_manager(sid) returns
None, and return/raise an appropriate HTTP error (e.g., 403 Forbidden or abort
with a JSON error) instead of continuing to dereference the object; ensure
helpers like get_accessible_server remain unchanged and that _get_connection
consistently enforces the access boundary.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current implementation of _get_connection and related handlers in erd/__init__.py
rg -n "connection_manager|_get_connection|None" web/pgadmin/tools/erd/__init__.py -A3 -B3
Length of output: 3255
@asheshv, the _get_connection function still dereferences manager without a None guard (Line 639–641). If connection_manager(sid) returns None for an unauthorized server, manager.connection(...) will raise an AttributeError and produce a 500 for /initialize, /prequisite, /sql, and the socket tables handler.
The close handler already does this correctly (Line 808), so the same pattern is needed inside _get_connection:
def _get_connection(sid, did, trans_id, db_name=None):
manager = get_driver(PG_DEFAULT_DRIVER).connection_manager(sid)
if manager is None:
raise ConnectionLost(sid, None, trans_id)
try:
conn = manager.connection(
conn_id=trans_id,
auto_reconnect=True,
use_binary_placeholder=True,
**({"database": db_name} if db_name is not None else {"did": did})
)
status, msg = conn.connect()
if not status:
app.logger.error(msg)
raise ConnectionLost(sid, conn.db, trans_id)
return conn
except Exception as e:
app.logger.error(e)
raiseSince ConnectionLost is already caught and translated into a 428 response in initialize_erd, and the other callers (prequisite, sql, tables) let exceptions propagate, you may want to handle the None case more explicitly in those callers (e.g., return a 410 JSON response), but at minimum the None check prevents the AttributeError 500.
🧠 Learnings used
Learnt from: dpage
Repo: pgadmin-org/pgadmin4 PR: 9737
File: web/pgadmin/tools/sqleditor/tests/test_nlq_chat.py:118-122
Timestamp: 2026-03-16T10:39:46.167Z
Learning: For Python tests using unittest.mock.patch, patch at the source module when the imported symbol is brought in inside a function (from x import y). The established rule “patch at the use/import site, not the definition site” only applies to module-level imports. If a from x import y statement appears inside a function body, it rebinds y on every call after patch.start(), so patching the source module (e.g., 'pgadmin.llm.chat.chat_with_database') is required. This pattern is demonstrated in web/pgadmin/tools/sqleditor/tests/test_nlq_chat.py for chat_with_database.
Learnt from: RohitBhati8269
Repo: pgadmin-org/pgadmin4 PR: 9749
File: web/pgadmin/misc/bgprocess/process_executor.py:210-225
Timestamp: 2026-03-16T11:35:24.319Z
Learning: In Python 3, sys.getdefaultencoding() always returns 'utf-8' and is not locale-dependent. Do not flag or fix code for encoding issues based on sys.getdefaultencoding() returning a non-UTF-8 value. If encoding behavior is locale-specific, rely on locale.getpreferredencoding() or, in some cases, sys.getfilesystemencoding(). Use those values for locale-aware I/O decisions, not getdefaultencoding(). This guidance applies to Python files across the codebase; ensure reviewers check for true locale-specific encoding handling where relevant, and avoid noisy encodings warnings based on getdefaultencoding().
Contributor
Author
There was a problem hiding this comment.
Fixed. Added if manager is None guard in _get_connection().
asheshv
force-pushed
the
fix/data-isolation-bugs
branch
from
b5b16d8 to
35a46de
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/pgadmin/tools/sqleditor/__init__.py (1)
230-239:⚠️ Potential issue | 🟠 MajorApply shared-server overrides here before reading saved credentials.
initialize_viewdata()now accepts shared servers viaget_server(sid), but it still readsserver.passwordfrom the baseServerrow._init_sqleditor()already resolvesSharedServeroverrides first; this path should do the same or shared-server reconnects can use the owner's saved credentials instead of the current user's copy.Possible fix
server = get_server(sid) if server is None: return make_json_response( status=410, success=0, errormsg=gettext("Could not find the required server.") ) + if server.shared and server.user_id != current_user.id: + from pgadmin.browser.server_groups.servers import ServerModule + shared_server = ServerModule.get_shared_server(server, sgid) + if shared_server is not None: + server = ServerModule.get_shared_server_properties( + server, shared_server + ) if kwargs.get('password', None) is None: kwargs['encpass'] = server.password🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/__init__.py` around lines 230 - 239, initialize_viewdata is reading credentials from the base Server row (server.password) instead of the resolved SharedServer overrides; update the logic so the resolved server returned by get_server(sid) (the same resolution used by _init_sqleditor()) is used when populating kwargs['encpass']. In practice, apply the shared-server override resolution before checking kwargs.get('password') and assign kwargs['encpass'] from the already-resolved server object (not the base Server row), or reuse the override-resolution routine from _init_sqleditor() to obtain the correct server instance prior to reading its password.web/pgadmin/browser/server_groups/servers/databases/__init__.py (1)
553-585:⚠️ Potential issue | 🟠 MajorGuard the server lookup before dereferencing
manager.Line 556 and Line 593 still call
connection_manager(sid)/manager.connection(...)before any not-found handling. If the sid is now inaccessible andconnection_manager()returnsNone, both endpoints still fail with a 500 before the newget_server(sid)fallback is reached. Please fail fast here the same way the other server-gated routes in this PR do.Also applies to: 588-609
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/browser/server_groups/servers/databases/__init__.py` around lines 553 - 585, In the connect method, guard the get_driver(...).connection_manager(sid) result before calling manager.connection or dereferencing manager: after assigning manager = get_driver(PG_DEFAULT_DRIVER).connection_manager(sid) check if manager is falsy and return the same not-found/fail-fast response used elsewhere (use get_server(sid) to produce the fallback message) instead of proceeding; likewise apply the same guard pattern before any manager.connection(...) calls in the surrounding database handlers (the other block around the existing get_server(sid) usage) so you never call manager.connection or manager methods when connection_manager(sid) returned None.
♻️ Duplicate comments (2)
web/pgadmin/tools/sqleditor/__init__.py (1)
522-527:⚠️ Potential issue | 🟠 MajorReturn the normal 410 not-found response here, not a 500.
This branch now correctly stops
_init_sqleditor()when the server is inaccessible, butinternal_server_error()still reports it as a server failure. The other changed sqleditor routes usestatus=410, so this path will still diverge in client handling.Possible fix
server = get_server(sid) if server is None: - return True, internal_server_error( - errormsg=gettext( - "Could not find the required server.") - ), '', '' + return True, make_json_response( + status=410, + success=0, + errormsg=gettext("Could not find the required server.") + ), '', ''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/tools/sqleditor/__init__.py` around lines 522 - 527, The current branch returning internal_server_error when server = get_server(sid) is None wrongly produces a 500; change the return so it sends a 410 Not Found status like the other sqleditor routes. Locate the server lookup around server = get_server(sid) in __init__.py (the early return in _init_sqleditor) and replace the internal_server_error call with the not-found response used elsewhere (or call internal_server_error/not_found_error with status=410) so the API returns HTTP 410 for an inaccessible/missing server.web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py (1)
98-104:⚠️ Potential issue | 🟡 MinorMissing response validation before dereferencing.
Unlike
ServerDataIsolationGetTestCase.setUp(lines 50-52), this test lacks validation before accessingresponse_data['node']['_id']. If server creation fails, this will raise a noisyKeyErrorinstead of a clear assertion failure.💚 Proposed fix to add validation
response = self.tester.post( url, data=json.dumps(self.server), content_type='html/json' ) + self.assertEqual(response.status_code, 200) response_data = json.loads(response.data.decode('utf-8')) + self.assertIn('node', response_data) self.server_id = response_data['node']['_id']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py` around lines 98 - 104, The test currently dereferences response_data['node']['_id'] without validating the response; update the test around the tester.post call to first assert the response status and that response_data contains the 'node' key (and that 'node' contains '_id') before assigning self.server_id. Specifically, after calling self.tester.post(...) and parsing response_data, add assertions similar to those in ServerDataIsolationGetTestCase.setUp (check response.status_code/response.status and 'node' in response_data and '_id' in response_data['node']) so failures produce clear assertion errors instead of KeyError when assigning self.server_id.
🧹 Nitpick comments (1)
web/pgadmin/browser/server_groups/servers/__init__.py (1)
2112-2146: Dead code: shared server handling is unreachable afteronly_owned=True.Since
get_server(sid, only_owned=True)only returns servers whereuser_id == current_user.id, the condition at line 2122 (server.shared and server.user_id != current_user.id) will always evaluate toFalse. The entireifblock at lines 2122-2133 and the repeated check at lines 2135-2136 and 2143-2144 are unreachable.Consider removing this dead code to avoid confusion:
♻️ Proposed simplification
try: # Owner-only: clearing saved password must not modify # another user's server record via shared access. server = get_server(sid, only_owned=True) - shared_server = None if server is None: return make_json_response( success=0, info=self.not_found_error_msg() ) - if server.shared and server.user_id != current_user.id: - shared_server = SharedServer.query.filter_by( - name=server.name, user_id=current_user.id, - servergroup_id=gid, osid=server.id).first() - - if shared_server is None: - return make_json_response( - success=0, - info=gettext("Could not find the required server.") - ) - server = ServerModule. \ - get_shared_server_properties(server, shared_server) - - if server.shared and server.user_id != current_user.id: - setattr(shared_server, 'password', None) - else: - setattr(server, 'password', None) + setattr(server, 'password', None) # If password was saved then clear the flag also # 0 is False in SQLite db if server.save_password: - if server.shared and server.user_id != current_user.id: - setattr(shared_server, 'save_password', 0) - else: - setattr(server, 'save_password', 0) + setattr(server, 'save_password', 0) db.session.commit()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/browser/server_groups/servers/__init__.py` around lines 2112 - 2146, The code path handling shared servers is unreachable because get_server(sid, only_owned=True) guarantees server.user_id == current_user.id; remove the dead shared-server branches and variables (shared_server, the if blocks checking server.shared and server.user_id != current_user.id, and the calls to SharedServer and ServerModule.get_shared_server_properties) and simplify to operate directly on server (set password to None and clear save_password flag when server.save_password is truthy, using 0 for SQLite) while keeping the existing not-found response and behavior of get_server and ServerModule references only where still needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/browser/server_groups/servers/utils.py`:
- Around line 658-677: The loop currently emits disconnect-psql for every socket
id stored under current_app.config['sid_soid_mapping'][str(server.id)], which
will disconnect other users on shared servers; change the logic to only target
sockets for the current owning user (or skip shared servers). Concretely, update
the code around get_user_server_query(), the server loop, and the
sid_soid_mapping lookup so you either (A) skip servers that are shared (detect
via the server ownership attribute on the Server model used in
get_user_server_query()) and only emit for non-shared servers, or (B) make the
mapping user-scoped by keying into current_app.config['sid_soid_mapping'] with a
composite key including the current user's id (e.g. server id + user id) and
only emit to those socket ids; ensure the condition that looks up
sid_soid_mapping uses the new per-user key (and adjust sio.emit calls
accordingly) before calling manager.release().
In `@web/pgadmin/browser/server_groups/tests/test_sg_data_isolation.py`:
- Around line 36-50: Initialize self.sg_id = None at the start of setUp() so
failures during the POST/JSON parse don't leave it undefined, and update
tearDown() to only attempt cleanup when self.sg_id is not None; specifically, in
the test class containing setUp and tearDown referenced above, add self.sg_id =
None as the first statement in setUp(), and guard any cleanup logic that uses
self.sg_id (e.g., deleting the server group) with a conditional check to skip
cleanup if it's None; apply the same initialization/guard pattern to the other
similar setup block noted (the one around lines 68-73).
In `@web/pgadmin/misc/workspaces/__init__.py`:
- Around line 230-232: Don't early-return when get_server(sid) yields None; that
skips best-effort cleanup for adhoc rows. Remove the immediate "return" after
server = get_server(sid) and ensure delete_adhoc_servers(sid) (and any manager
cleanup logic that follows) is executed regardless of whether server is None;
only guard usages that dereference server (e.g., calling methods/attributes on
server) with an if server: check so the cleanup still runs for
deleted/reassigned adhoc rows.
In `@web/pgadmin/tools/schema_diff/__init__.py`:
- Around line 521-526: The code currently treats get_schemas() returning None as
both "server not found" and generic failure, losing error granularity; update
get_schemas() (and its callers used in compare_database and the other two spots)
to return/raise distinct failure signals (e.g., raise specific exceptions like
ServerNotFoundError or PermissionError, or return a (result, error) tuple or a
distinct sentinel such as False for auth/server-missing and None only for
genuine empty results), then change the compare_database handler (and the other
occurrences around schema_result handling) to detect those distinct signals and
emit socketio events with accurate messages (use request.sid and
SOCKETIO_NAMESPACE as before, but emit different events/messages for
auth/server-missing vs runtime failures) so permission misses are not rewritten
as "Could not find the required server."
- Around line 34-35: The module-defined route function get_server(sid, did) is
shadowing the imported helper from pgadmin.utils.server_access and breaking
calls that expect the single-argument server-access helper; fix by aliasing the
import (e.g. import get_server as get_accessible_server) and update all internal
call sites that currently call get_server(sid) — including inside the route
itself, connect_server(), connect_database(), databases(),
check_version_compatibility(), and get_schemas() — to call
get_accessible_server(sid) so the correct helper is used while leaving the route
function name unchanged.
---
Outside diff comments:
In `@web/pgadmin/browser/server_groups/servers/databases/__init__.py`:
- Around line 553-585: In the connect method, guard the
get_driver(...).connection_manager(sid) result before calling manager.connection
or dereferencing manager: after assigning manager =
get_driver(PG_DEFAULT_DRIVER).connection_manager(sid) check if manager is falsy
and return the same not-found/fail-fast response used elsewhere (use
get_server(sid) to produce the fallback message) instead of proceeding; likewise
apply the same guard pattern before any manager.connection(...) calls in the
surrounding database handlers (the other block around the existing
get_server(sid) usage) so you never call manager.connection or manager methods
when connection_manager(sid) returned None.
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 230-239: initialize_viewdata is reading credentials from the base
Server row (server.password) instead of the resolved SharedServer overrides;
update the logic so the resolved server returned by get_server(sid) (the same
resolution used by _init_sqleditor()) is used when populating kwargs['encpass'].
In practice, apply the shared-server override resolution before checking
kwargs.get('password') and assign kwargs['encpass'] from the already-resolved
server object (not the base Server row), or reuse the override-resolution
routine from _init_sqleditor() to obtain the correct server instance prior to
reading its password.
---
Duplicate comments:
In
`@web/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.py`:
- Around line 98-104: The test currently dereferences
response_data['node']['_id'] without validating the response; update the test
around the tester.post call to first assert the response status and that
response_data contains the 'node' key (and that 'node' contains '_id') before
assigning self.server_id. Specifically, after calling self.tester.post(...) and
parsing response_data, add assertions similar to those in
ServerDataIsolationGetTestCase.setUp (check response.status_code/response.status
and 'node' in response_data and '_id' in response_data['node']) so failures
produce clear assertion errors instead of KeyError when assigning
self.server_id.
In `@web/pgadmin/tools/sqleditor/__init__.py`:
- Around line 522-527: The current branch returning internal_server_error when
server = get_server(sid) is None wrongly produces a 500; change the return so it
sends a 410 Not Found status like the other sqleditor routes. Locate the server
lookup around server = get_server(sid) in __init__.py (the early return in
_init_sqleditor) and replace the internal_server_error call with the not-found
response used elsewhere (or call internal_server_error/not_found_error with
status=410) so the API returns HTTP 410 for an inaccessible/missing server.
---
Nitpick comments:
In `@web/pgadmin/browser/server_groups/servers/__init__.py`:
- Around line 2112-2146: The code path handling shared servers is unreachable
because get_server(sid, only_owned=True) guarantees server.user_id ==
current_user.id; remove the dead shared-server branches and variables
(shared_server, the if blocks checking server.shared and server.user_id !=
current_user.id, and the calls to SharedServer and
ServerModule.get_shared_server_properties) and simplify to operate directly on
server (set password to None and clear save_password flag when
server.save_password is truthy, using 0 for SQLite) while keeping the existing
not-found response and behavior of get_server and ServerModule references only
where still needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5197c81-eec3-4cbe-a8c5-aaf3a5ca0583
📒 Files selected for processing (18)
web/migrations/versions/add_user_id_to_debugger_func_args_.pyweb/pgadmin/browser/server_groups/__init__.pyweb/pgadmin/browser/server_groups/servers/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/__init__.pyweb/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.pyweb/pgadmin/browser/server_groups/servers/tests/test_server_data_isolation.pyweb/pgadmin/browser/server_groups/servers/utils.pyweb/pgadmin/browser/server_groups/tests/test_sg_data_isolation.pyweb/pgadmin/misc/workspaces/__init__.pyweb/pgadmin/tools/debugger/__init__.pyweb/pgadmin/tools/erd/__init__.pyweb/pgadmin/tools/import_export/__init__.pyweb/pgadmin/tools/psql/__init__.pyweb/pgadmin/tools/schema_diff/__init__.pyweb/pgadmin/tools/sqleditor/__init__.pyweb/pgadmin/utils/__init__.pyweb/pgadmin/utils/driver/psycopg3/__init__.pyweb/pgadmin/utils/server_access.py
✅ Files skipped from review due to trivial changes (3)
- web/pgadmin/tools/debugger/init.py
- web/pgadmin/tools/psql/init.py
- web/pgadmin/tools/erd/init.py
🚧 Files skipped from review as they are similar to previous changes (6)
- web/pgadmin/utils/driver/psycopg3/init.py
- web/pgadmin/browser/server_groups/servers/databases/schemas/views/init.py
- web/pgadmin/browser/server_groups/init.py
- web/migrations/versions/add_user_id_to_debugger_func_args_.py
- web/pgadmin/utils/init.py
- web/pgadmin/tools/import_export/init.py
asheshv
force-pushed
the
fix/data-isolation-bugs
branch
2 times, most recently
from
022ff77 to
3e0dc28
Compare
In server mode, multiple users share one pgAdmin instance. Several code paths loaded objects belonging to all users without filtering, allowing cross-user information disclosure. Adds centralized get_server() helper with only_owned parameter for write operations, scopes all server/group queries, adds access check in connection_manager(), and includes Alembic migration for DebuggerFunctionArguments user_id column.
Mixin providing Model.for_user(**kwargs) as the standard scoped query entry point. Applied to 11 user-scoped models. Converted all Process query callsites. Updated BatchProcess test mocks.
asheshv
force-pushed
the
fix/data-isolation-bugs
branch
from
3e0dc28 to
f097f70
Compare
Three test classes verifying server/group isolation and shared server access. Uses create_user_wise_test_client pattern, SERVER_MODE only.
asheshv
force-pushed
the
fix/data-isolation-bugs
branch
from
f097f70 to
b44713f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UserScopedMixin.for_user()to model layer as the standard pattern for user-scoped queriesChanges
Data isolation fixes:
get_accessible_server(),get_accessible_server_group(),get_server_groups_for_user(),get_user_server_query()helpers inutils/server_access.pyServer.query.filter_by(id=sid)callsites with access-checked helper across servers, sqleditor, schema_diff, erd, psql, import_export, views, workspaces, debuggerconnection_manager()— the security boundary for allcheck_precondition-decorated endpointsServerGroup.query.all()to current user + shared server groupsuser_idtoServerGroup.properties()andnodes(gid)with shared group supportor_(Server.user_id, Server.shared)listing patterns withget_user_server_query()user_idtoDebuggerFunctionArgumentsmodel (composite PK) with Alembic migration; SCHEMA_VERSION bumped to 50disconnect_from_all_servers(),delete_adhoc_servers(),get_servers_with_saved_passwords()to current userserver(user_id),server(servergroup_id),sharedserver(user_id),sharedserver(osid),servergroup(user_id)ca00ec32581bto use raw SQL (model import broke on fresh databases)UserScopedMixin:
Model.for_user(**kwargs)as the standard scoped query entry pointuser_idanduidcolumn naming conventionsProcess.query.filter_by(user_id=...)callsites toProcess.for_user()Tests:
ServerGroupIsolationTestCase— non-admin cannot fetch another user's server groupServerDataIsolationGetTestCase— non-admin cannot access another user's private serverSharedServerAccessTestCase— shared servers remain accessible cross-user (regression test)Notes
Server.shared=Truemeans visible to all authenticated users;SharedServerrecords are for per-user customization, not access controlSERVER_MODE=TruewithpgAdmin4_test_non_admin_credentialsconfiguredTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores