fix: SharedServer feature parity columns and write guards#9835
fix: SharedServer feature parity columns and write guards#9835
Conversation
Add passexec_cmd, passexec_expiration, kerberos_conn, tags, and post_connection_sql columns to SharedServer so non-owners can have per-user customizations overlaid from their own record instead of inheriting (or suppressing) the owner's values. Security hardening: - Non-owners blocked from setting passexec_cmd, passexec_expiration, post_connection_sql, db_res, db_res_type via _owner_only_fields guard - Driver-level suppression retained for connection layer defense-in-depth - Re-suppression after manager.update() in connect() endpoint - SharedServer creation defaults dangerous fields to None/False Migration improvements: - Orphan cleanup: delete SharedServer rows referencing deleted Servers - Column-exists guard prevents failures on migration re-runs - Unique constraint idempotency guard via get_unique_constraints() Other fixes: - Null guard in get_shared_server_properties() for None sharedserver - Unit tests updated for overlay semantics with full field coverage
WalkthroughExtended the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 161-164: The migration defines the 'kerberos_conn' column with
server_default='false'; change this to use SQLAlchemy's portable boolean literal
by setting server_default=sa.false() on the sa.Column(...) for 'kerberos_conn'
(and ensure sa.false is available from the imported sa namespace if not
already). This ensures the default renders as a proper SQL boolean literal
across DB backends.
In `@web/pgadmin/browser/server_groups/servers/__init__.py`:
- Around line 1182-1184: The current truthiness check for passexec_expiration
collapses an explicit 0 to None; change the conditional to test explicitly for
None (e.g. use "if server.passexec_expiration is not None else None") so that a
value of 0 is preserved when building the properties dict for
passexec_expiration and will round-trip correctly through the properties API.
- Around line 230-231: The update path currently computes tag deltas using
server.tags (the owner's tags) which causes non-owner SharedServer.tags to be
rebuilt from the owner; change the merge base so that when handling a non-owner
update (where SharedServer exists and you assign server.kerberos_conn =
sharedserver.kerberos_conn and server.tags = sharedserver.tags) you compute tag
diffs against sharedserver.tags (i.e. use SharedServer.tags as the base) instead
of server.tags inside update(), and apply only the delta to the non-owner
SharedServer record to avoid overwriting its existing tags.
In `@web/pgadmin/model/__init__.py`:
- Around line 563-569: The SCHEMA_VERSION constant was not incremented after
adding new SharedServer columns (passexec_cmd, passexec_expiration,
kerberos_conn, tags, post_connection_sql), so update the module's SCHEMA_VERSION
(e.g., from 50 to 51) where it is defined so upgrades detect and run the
migration for the SharedServer changes; ensure the new version is used by
migration/upgrade logic and any tests referencing SCHEMA_VERSION are updated
accordingly.
🪄 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: 49a1e807-6d76-4299-b568-5be7ba3c9c6e
📒 Files selected for processing (5)
web/migrations/versions/add_user_id_to_debugger_func_args_.pyweb/pgadmin/browser/server_groups/servers/__init__.pyweb/pgadmin/browser/server_groups/servers/tests/test_shared_server_unit.pyweb/pgadmin/model/__init__.pyweb/pgadmin/utils/driver/psycopg3/__init__.py
| ('kerberos_conn', | ||
| sa.Column('kerberos_conn', sa.Boolean(), | ||
| nullable=False, | ||
| server_default='false')), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In SQLAlchemy/Alembic, for a Boolean column, does server_default='false'render as a quoted string literal or a SQL boolean literal, and isserver_default=sa.text('false')orsa.false() the recommended portable form?
💡 Result:
server_default='false'is treated as a SQL expression string, not a Python boolean. In DDL it will typically render asDEFAULT false(unquoted), i.e. notDEFAULT 'false'—SQLAlchemy only applies string-literal quoting rules when the default string itself represents a quoted literal value. [1]server_default=sa.text('false')likewise rendersDEFAULT false; it’s just the explicit “this is SQL text” form (SQLAlchemy docs advisetext()over raw strings for literal SQL defaults). [2]- For the most portable/cross-dialect boolean server default, prefer
server_default=sa.false()(orsa.true()), since SQLAlchemy can adapt true/false constants on backends that don’t support native boolean literals. [3] (This is also the common Alembic/SQLAlchemy migration pattern you’ll see recommended in practice. [4])
Sources: [1] SQLAlchemy migration note on string server_default quoting behavior (docs.sqlalchemy.org); [2] SQLAlchemy defaults docs recommending text() for literal SQL in server_default (docs.sqlalchemy.org); [3] SQLAlchemy false() constant portability across backends (docs.sqlalchemy.org); [4] Alembic/SQLAlchemy usage example recommending sa.false() (stackoverflow.com).
Citations:
- 1: https://docs.sqlalchemy.org/21/changelog/migration_11.html
- 2: https://docs.sqlalchemy.org/20/core/defaults.html
- 3: https://docs.sqlalchemy.org/21/core/sqlelement.html?utm_source=openai
- 4: https://stackoverflow.com/questions/62212263/alembic-doesnt-recognize-false-default-value?utm_source=openai
Use sa.false() for the kerberos_conn server default.
server_default='false' works, but sa.false() is the recommended portable form in Alembic/SQLAlchemy. It ensures the default renders as a proper SQL boolean literal across different database backends (including SQLite) rather than relying on string-to-boolean coercion.
🔧 Suggested fix
('kerberos_conn',
sa.Column('kerberos_conn', sa.Boolean(),
nullable=False,
- server_default='false')),
+ server_default=sa.false())),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ('kerberos_conn', | |
| sa.Column('kerberos_conn', sa.Boolean(), | |
| nullable=False, | |
| server_default='false')), | |
| ('kerberos_conn', | |
| sa.Column('kerberos_conn', sa.Boolean(), | |
| nullable=False, | |
| server_default=sa.false())), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/migrations/versions/add_user_id_to_debugger_func_args_.py` around lines
161 - 164, The migration defines the 'kerberos_conn' column with
server_default='false'; change this to use SQLAlchemy's portable boolean literal
by setting server_default=sa.false() on the sa.Column(...) for 'kerberos_conn'
(and ensure sa.false is available from the imported sa namespace if not
already). This ensures the default renders as a proper SQL boolean literal
across DB backends.
| server.kerberos_conn = sharedserver.kerberos_conn | ||
| server.tags = sharedserver.tags |
There was a problem hiding this comment.
Use sharedserver.tags as the merge base for non-owner updates.
Now that tags is overlaid from SharedServer, update() still builds tag deltas from server.tags on the owner row. Editing an existing shared copy can therefore rebuild the list from the owner's tags and overwrite the non-owner's current SharedServer.tags.
🔧 Suggested fix
- self.update_tags(data, server)
+ self.update_tags(data, sharedserver or server)🤖 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 230 -
231, The update path currently computes tag deltas using server.tags (the
owner's tags) which causes non-owner SharedServer.tags to be rebuilt from the
owner; change the merge base so that when handling a non-owner update (where
SharedServer exists and you assign server.kerberos_conn =
sharedserver.kerberos_conn and server.tags = sharedserver.tags) you compute tag
diffs against sharedserver.tags (i.e. use SharedServer.tags as the base) instead
of server.tags inside update(), and apply only the delta to the non-owner
SharedServer record to avoid overwriting its existing tags.
| 'passexec_expiration': | ||
| server.passexec_expiration | ||
| if server.passexec_expiration and | ||
| not _is_non_owner(server) else None, | ||
| if server.passexec_expiration else None, |
There was a problem hiding this comment.
Keep 0 distinct from None for passexec_expiration.
This truthiness check collapses an explicit 0 to None, so that value will not round-trip through the properties API.
🔧 Suggested fix
- 'passexec_expiration':
- server.passexec_expiration
- if server.passexec_expiration else None,
+ 'passexec_expiration': server.passexec_expiration,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'passexec_expiration': | |
| server.passexec_expiration | |
| if server.passexec_expiration and | |
| not _is_non_owner(server) else None, | |
| if server.passexec_expiration else None, | |
| 'passexec_expiration': server.passexec_expiration, |
🤖 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 1182 -
1184, The current truthiness check for passexec_expiration collapses an explicit
0 to None; change the conditional to test explicitly for None (e.g. use "if
server.passexec_expiration is not None else None") so that a value of 0 is
preserved when building the properties dict for passexec_expiration and will
round-trip correctly through the properties API.
| passexec_cmd = db.Column(db.Text(), nullable=True) | ||
| passexec_expiration = db.Column(db.Integer(), nullable=True) | ||
| kerberos_conn = db.Column( | ||
| db.Boolean(), nullable=False, default=0 | ||
| ) | ||
| tags = db.Column(types.JSON) | ||
| post_connection_sql = db.Column(db.String(), nullable=True) |
There was a problem hiding this comment.
Bump SCHEMA_VERSION with these new SharedServer columns.
This changes the config DB schema, but SCHEMA_VERSION is still 50 in this module. Existing installs that gate upgrades off that version can miss the migration and keep running against a sharedserver table that does not have these fields yet.
🔧 Suggested fix
-SCHEMA_VERSION = 50
+SCHEMA_VERSION = 51🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/model/__init__.py` around lines 563 - 569, The SCHEMA_VERSION
constant was not incremented after adding new SharedServer columns
(passexec_cmd, passexec_expiration, kerberos_conn, tags, post_connection_sql),
so update the module's SCHEMA_VERSION (e.g., from 50 to 51) where it is defined
so upgrades detect and run the migration for the SharedServer changes; ensure
the new version is used by migration/upgrade logic and any tests referencing
SCHEMA_VERSION are updated accordingly.
1 participant
Summary
SharedServermodel (passexec_cmd,passexec_expiration,kerberos_conn,tags,post_connection_sql) for feature parity withServer, so non-owners get per-user values via overlay instead of inheriting/suppressing owner fieldspassexec_cmd,passexec_expiration,post_connection_sql,db_res,db_res_type) via_owner_only_fieldswrite guard in_set_valid_attr_value()passexec/post_connection_sqlonServerManagerfor non-owners as defense-in-depth, including re-suppression aftermanager.update()in theconnect()endpointget_shared_server_properties()forNonesharedserverBuilds on top of #9830.
Test plan
cd web && python regression/runtests.py --pkg browser.server_groups.servers --modules test_shared_server_unit— all unit tests passpassexec_cmdorpost_connection_sqlvia PUT to/browser/server/obj/passexec_cmdkerberos_conn,tags,post_connection_sqlvalues🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements