-
Notifications
You must be signed in to change notification settings - Fork 879
Exclude non-editable alias/expression columns from Query Tool UPDATE saves #10329
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
767c8b9
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 |
|---|---|---|
| @@ -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 |
|---|---|---|
|
|
@@ -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 %}; | ||
|
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 | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 8 --glob '*.py' --glob '*.js' \
'validate_maintenance_data|reindex_system|reindex_concurrently' \
web/pgadminRepository: pgadmin-org/pgadmin4 Length of output: 16560 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- validator ---'
sed -n '145,210p' web/pgadmin/tools/maintenance/__init__.py
printf '%s\n' '--- reindex controls ---'
sed -n '250,325p' web/pgadmin/tools/maintenance/static/js/maintenance.ui.js
printf '%s\n' '--- template ---'
sed -n '1,45p' web/pgadmin/tools/maintenance/templates/maintenance/sql/command.sql
printf '%s\n' '--- related tests ---'
rg -n -C 5 --glob '*.py' \
'reindex_system.*reindex_concurrently|reindex_concurrently.*reindex_system|SYSTEM.*CONCURRENTLY|CONCURRENTLY.*SYSTEM' \
web/pgadmin/tools/maintenanceRepository: pgadmin-org/pgadmin4 Length of output: 8532 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- request path ---'
sed -n '208,285p' web/pgadmin/tools/maintenance/__init__.py
printf '%s\n' '--- reindex applicability ---'
rg -n -C 10 --glob '*.js' \
'isApplicableForReindex' web/pgadmin/tools/maintenance/static/js/maintenance.ui.js
printf '%s\n' '--- maintenance data construction and rendering ---'
rg -n -C 8 --glob '*.py' --glob '*.js' \
'command.sql|render_template|maintenance_options|json.loads\(.*data' \
web/pgadmin/tools/maintenanceRepository: pgadmin-org/pgadmin4 Length of output: 17780 Reject the unsupported flag combination on the server. 🤖 Prompt for AI Agents |
||
| {% endif %} | ||
| {% endif %} | ||
| {% if data.op == "CLUSTER" %} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -649,9 +649,11 @@ def check_is_integer(value): | |
| "found for server '%s'" % server | ||
| ) | ||
| else: | ||
| errmsg = check_attrib("Username") | ||
| if errmsg: | ||
| return errmsg | ||
| if not obj.get("Username"): | ||
| return gettext( | ||
| "'Username' attribute not found for server '%s'" % | ||
| server | ||
| ) | ||
|
Comment on lines
+652
to
+656
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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 8 -P '\bload_database_servers\s*\(' --glob '*.py'
rg -n -C 8 'from_setup|validate_json_data' web/pgadmin/utils/__init__.pyRepository: pgadmin-org/pgadmin4 Length of output: 158 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(__init__\.py|.*server.*\.py|.*setup.*\.py)$' | head -200
printf '%s\n' '--- target file and symbols ---'
target='web/pgadmin/utils/__init__.py'
if [ -f "$target" ]; then
wc -l "$target"
rg -n -C 10 'load_database_servers|validate_json_data|from_setup|error_msg|new_server\.username' "$target"
else
printf 'missing: %s\n' "$target"
fi
printf '%s\n' '--- all load_database_servers references ---'
rg -n -C 12 'load_database_servers' --glob '*.py' . || true
printf '%s\n' '--- all validate_json_data references ---'
rg -n -C 8 'validate_json_data' --glob '*.py' . || trueRepository: pgadmin-org/pgadmin4 Length of output: 19975 🏁 Script executed: #!/bin/bash
set -u
target='web/pgadmin/utils/__init__.py'
printf '%s\n' '--- target status ---'
if [ -f "$target" ]; then
wc -l "$target"
sed -n '620,780p' "$target"
else
printf 'missing: %s\n' "$target"
fi
printf '%s\n' '--- exact symbol references ---'
rg -n -C 12 'load_database_servers|validate_json_data|from_setup|error_msg|new_server\.username' "$target" || true
rg -n -C 12 'load_database_servers' --glob '*.py' . || trueRepository: pgadmin-org/pgadmin4 Length of output: 26722 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- regular import caller ---'
sed -n '150,210p' web/pgadmin/tools/import_export_servers/__init__.py
printf '%s\n' '--- tests covering import and validation ---'
rg -n -C 8 'load_database_servers|validate_json_data|SharedUsername|Username.*not found|import' \
web/pgadmin --glob '*test*.py' | head -300 || true
printf '%s\n' '--- AST verifier for defaults, guard, and call arguments ---'
python3 - <<'PY'
import ast
from pathlib import Path
utils_path = Path("web/pgadmin/utils/__init__.py")
caller_path = Path("web/pgadmin/tools/import_export_servers/__init__.py")
utils = ast.parse(utils_path.read_text())
caller = ast.parse(caller_path.read_text())
load = next(
node for node in utils.body
if isinstance(node, ast.FunctionDef) and node.name == "load_database_servers"
)
defaults = load.args.defaults
param_names = [arg.arg for arg in load.args.args]
default_map = dict(zip(param_names[-len(defaults):], defaults))
print("load_database_servers defaults:",
{name: ast.unparse(value) for name, value in default_map.items()})
guard = next(
node for node in ast.walk(load)
if isinstance(node, ast.If)
and isinstance(node.test, ast.BoolOp)
and isinstance(node.test.op, ast.And)
and any(isinstance(v, ast.Name) and v.id == "from_setup"
for v in node.test.values)
)
print("validation guard:", ast.unparse(guard.test))
print("guard body:", [ast.unparse(stmt) for stmt in guard.body])
calls = []
for node in ast.walk(caller):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) \
and node.func.id == "load_database_servers":
calls.append(node)
for call in calls:
print("regular caller:", ast.unparse(call))
print("positional from_setup supplied:", len(call.args) >= 4)
print("keyword from_setup supplied:",
any(kw.arg == "from_setup" for kw in call.keywords))
# Model the exact username checks for representative non-shared values.
for value in (None, "", "alice"):
invalid = not bool(value)
print(f"non_shared Username={value!r}: validation_error={invalid}, "
f"persisted_username={value!r}")
PYRepository: pgadmin-org/pgadmin4 Length of output: 25870 Reject validation errors for regular imports.
🤖 Prompt for AI Agents |
||
|
|
||
| errmsg = check_attrib("MaintenanceDB") | ||
| if errmsg: | ||
|
|
||
| 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 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Trace callers that invoke execute_void() and then poll().
rg -n -C 4 --type py '\.execute_void\s*\(|\.poll\s*\(' web
# Inspect the cursor-state handoff and poll behavior.
sed -n '1088,1235p' web/pgadmin/utils/driver/psycopg3/connection.py
sed -n '1593,1644p' web/pgadmin/utils/driver/psycopg3/connection.pyRepository: pgadmin-org/pgadmin4 Length of output: 50376 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- execute_void and poll implementations ---'
rg -n -A 90 -B 25 'def execute_void|def poll|__async_cursor|release_async_cursor' \
web/pgadmin/utils/driver/psycopg3/connection.py
printf '%s\n' '--- focused sqleditor call paths ---'
sed -n '960,1020p' web/pgadmin/tools/sqleditor/__init__.py
sed -n '1108,1140p' web/pgadmin/tools/sqleditor/__init__.py
sed -n '2148,2182p' web/pgadmin/tools/sqleditor/__init__.py
printf '%s\n' '--- server-cursor test and related tests ---'
sed -n '1,130p' web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py
rg -n -C 8 'execute_void|poll|column_info|row_count|__async_cursor' \
web/pgadmin/utils/driver/psycopg3/testsRepository: pgadmin-org/pgadmin4 Length of output: 48123 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- cursor acquisition and cache ownership ---'
sed -n '680,812p' web/pgadmin/utils/driver/psycopg3/connection.py
sed -n '1068,1090p' web/pgadmin/utils/driver/psycopg3/connection.py
printf '%s\n' '--- transaction-control call paths around subsequent polling ---'
sed -n '120,185p' web/pgadmin/tools/sqleditor/utils/start_running_query.py
sed -n '930,1015p' web/pgadmin/tools/sqleditor/__init__.py
rg -n -C 12 'execute_void\("(BEGIN|COMMIT|ROLLBACK)|execute_void\(sql\)' \
web/pgadmin/tools web/pgadmin/utils/driver/psycopg3/tests
printf '%s\n' '--- test fixtures for private async cursor state ---'
rg -n -C 10 '_Connection__async_cursor|poll\(' \
web/pgadmin/utils/driver/psycopg3/tests web/pgadmin/tools/sqleditor/testsRepository: pgadmin-org/pgadmin4 Length of output: 45618 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- sqleditor poll response handling ---'
rg -n -A 120 -B 20 'def poll\(' web/pgadmin/tools/sqleditor
rg -n -C 12 "'column_info'|'rows_fetched_to'|'result': result|Messages" \
web/pgadmin/tools/sqleditor/__init__.py web/pgadmin/tools/sqleditor
printf '%s\n' '--- focused state transition verifier ---'
python3 - <<'PY'
class Cursor:
def __init__(self, name, closed=False, description=None, rowcount=0,
rows=None):
self.name = name
self.closed = closed
self.description = description
self.rowcount = rowcount
self.rows = rows or []
def get_rowcount(self):
return self.rowcount
def fetchall(self, _tupples=True):
return self.rows
def nextset(self):
return False
class ConnectionModel:
def __init__(self, cached_cursor):
self.async_cursor = cached_cursor
self.column_info = [{'name': 'x'}]
self.row_count = 1
def execute_void_current_branch(self, plain_cursor):
# The branch shown in the review: local replacement only.
cur = plain_cursor
self.column_info = None
self.row_count = 0
return cur
def poll(self, no_result=False):
cur = self.async_cursor
if not cur or cur.closed:
return False, 'Cursor could not be found'
result = None
self.row_count = 0
self.column_info = None
if cur.description is not None:
self.column_info = cur.description
self.row_count = cur.get_rowcount()
if not no_result and cur.get_rowcount() > 0:
result = cur.fetchall()
while cur.nextset():
pass
return True, result
old = Cursor('server', description=[{'name': 'x'}], rowcount=1,
rows=[('stale',)])
plain = Cursor('plain', description=None, rowcount=0)
model = ConnectionModel(old)
model.execute_void_current_branch(plain)
status, result = model.poll()
print({
'cached_cursor_after_execute_void': model.async_cursor.name,
'poll_status': status,
'poll_result': result,
'column_info_after_poll': model.column_info,
'row_count_after_poll': model.row_count,
})
assert model.async_cursor is old
assert status is True
assert result == [('stale',)]
assert model.column_info == [{'name': 'x'}]
assert model.row_count == 1
PYRepository: pgadmin-org/pgadmin4 Length of output: 50376 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- connection type and cursor classes ---'
rg -n -C 8 'async_|AsyncConnection|psycopg\.connect|server_cursor_factory|class Async' \
web/pgadmin/utils/driver/psycopg3/connection.py \
web/pgadmin/utils/driver/psycopg3/cursor.py
printf '%s\n' '--- poll status constants and response branches ---'
rg -n -C 8 'ASYNC_OK|TX_STATUS|transaction_status|column_info' \
web/pgadmin/tools/sqleditor/__init__.py \
web/pgadmin/utils/driver/psycopg3/connection.py
printf '%s\n' '--- exact verifier output ---'
log=/tmp/coderabbit-shell-logs/shell-output-2bmyyW
rg -n -A 20 -B 5 'focused state transition verifier|cached_cursor_after_execute_void|Traceback|AssertionError' "$log" || trueRepository: pgadmin-org/pgadmin4 Length of output: 50377 Keep
📍 Affects 2 files
🤖 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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check the original request fields before validation mutates the request.
@validate_requestruns beforeupdate._validate_rolemembersaddsrol_members_listand related derived keys toself.request. A valid request that contains onlyrolmembersthen fails this subset check and returns 403.Store the client-supplied field names before validation. Use that stored set for the membership-only restriction. Add a regression test that executes the decorated
updatepath with arolmembers-only payload.Proposed fix
def wrap(self, **kwargs): if request.data: data = json.loads(request.data) else: data = dict() ... + self.request_fields = set(data) invalid_msg_arr = [ ... ]🤖 Prompt for AI Agents