Load predefined users from a JSON file through command line. #9229#9652
Load predefined users from a JSON file through command line. #9229#9652khushboovashi wants to merge 5 commits intopgadmin-org:masterfrom
Conversation
WalkthroughAdds a JSON-driven bulk user import CLI ( Changes
Sequence DiagramsequenceDiagram
actor CLI as User / CLI
participant File as JSON File
participant Parser as JSON Parser
participant Importer as ManageUsers.load_users
participant Roles as ManageRoles
participant DB as Database
CLI->>File: open input_file
File-->>Parser: read & parse JSON
Parser-->>Importer: users[] list
loop per user entry
Importer->>Importer: validate fields & password rules
Importer->>DB: lookup user by username/email
DB-->>Importer: exists? (yes/no)
alt not exists
Importer->>Roles: resolve role names to IDs
Roles-->>Importer: role IDs
Importer->>DB: create user record
DB-->>Importer: created
else exists
Importer-->>Importer: mark skipped
end
end
Importer-->>CLI: report counts (created, skipped, errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🧹 Nitpick comments (3)
web/setup.py (3)
246-257: No validation ofauth_sourcevalues from user-supplied JSON.The
auth_sourcefield is taken verbatim from the JSON (line 247). If a user provides an unrecognized value (e.g.,"Internal"capitalized, or a typo like"interanl"), it silently passes through andcreate_userwill attempt to create the user with an invalid auth source. Consider validating against the known constants (INTERNAL,LDAP,OAUTH2,KERBEROS,WEBSERVER) and reporting an error for unrecognized values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 246 - 257, The code assigns auth_source directly from user_entry into user_data without validating it; update the block that reads auth_source (the user_entry.get('auth_source', INTERNAL) and the user_data dict) to validate the value against the allowed constants (INTERNAL, LDAP, OAUTH2, KERBEROS, WEBSERVER) and normalize casing if desired, and if the provided value is not one of these, raise or return a clear error before calling create_user so invalid auth_source values (e.g., "Internal" or typos) are rejected and only recognized constants are used.
209-211:open()without explicit encoding.
open(file_path)uses the platform default encoding, which may not be UTF-8 on all systems (notably Windows). Since JSON is typically UTF-8:- with open(file_path) as f: + with open(file_path, encoding='utf-8') as f:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 209 - 211, The file opens JSON files using open(file_path) without an explicit encoding; update the code that reads into jsonlib.load to open the file with an explicit UTF-8 encoding (i.e., pass encoding="utf-8" to open) so reads are deterministic across platforms—locate the try block that uses file_path and jsonlib.load and change the open call accordingly.
289-295: Useconfig.PASSWORD_LENGTH_MINinstead of hardcoded6for consistency with the rest of the codebase.Password validation elsewhere in the codebase (e.g.,
web/pgadmin/setup/user_info.py,web/pgadmin/browser/__init__.py) consistently usesconfig.PASSWORD_LENGTH_MIN. Hardcoding6here means this validation will drift if the configured minimum changes.Since
configis already imported, update the check and error message:♻️ Proposed fix
if auth_source == INTERNAL: - if len(user_data['newPassword']) < 6: + if len(user_data['newPassword']) < \ + config.PASSWORD_LENGTH_MIN: print(f"Skipping user '{user_data['username']}': " - f"password must be at least 6 characters") + f"password must be at least " + f"{config.PASSWORD_LENGTH_MIN} characters")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 289 - 295, Replace the hardcoded minimum password length 6 with the configured constant by using config.PASSWORD_LENGTH_MIN in the validation and the error message: in the block that checks auth_source == INTERNAL and inspects user_data['newPassword'] (and increments error_count/continues), change the numeric literal to config.PASSWORD_LENGTH_MIN and update the printed string to interpolate that constant so the message reflects the configured minimum.
🤖 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/setup.py`:
- Around line 166-170: The load_users command currently accepts a json parameter
but never uses it; update the load_users function to honor the json flag (or
remove the parameter) — specifically, in the load_users function implement
conditional output: after loading users, if json is True serialize the resulting
users list to JSON (matching the format used by the get-users command) and print
to stdout (or write to the same destination get-users uses for JSON), otherwise
keep the existing human-readable output; keep the `@update_sqlite_path` decorator
as-is and use the existing user-loading logic in load_users to produce the
object you will JSON-serialize.
- Around line 202-219: Remove the redundant print of the exception in the
unquote error path: when unquote(input_file) raises, stop calling print(str(e))
and instead directly return _handle_error(str(e), True) (same behavior as the
JSON error branches). Update the try/except around unquote to catch Exception as
e, assign file_path = unquote(input_file) in the try, and in the except simply
return _handle_error(str(e), True) (references: unquote, input_file, file_path,
_handle_error).
- Around line 240-244: The print statement in the user import block (where
user_entry is validated) uses an f-string with no placeholders and doesn't
identify which entry failed; update the message to include context (e.g., the
user_entry contents or its index) so failures are debuggable and to satisfy Ruff
F541. Locate the validation around the user_entry variable in web/setup.py (the
block that checks 'username' and 'email') and change the print to include either
the current index from an enumerate or the user_entry dict (or both) in the
output, and keep incrementing error_count and continue as before.
- Around line 270-285: Replace calls to ManageUsers.get_user and
ManageRoles.get_role inside load_users with direct ORM queries against the User
and Role models using the existing app context (avoid calling
create_app()/test_request_context inside ManageUsers/ManageRoles to eliminate
per-user app creation); also change the f-string "f\"Skipping user:
missing...\"" to a plain string, replace the hardcoded password min length 6
with config.PASSWORD_LENGTH_MIN, open files with explicit encoding (e.g.
open(file_path, encoding="utf-8")), remove the redundant explicit print of
errors so only _handle_error prints/logs them, and remove the unused json
parameter from the function signature and any callers. Ensure you reference
ManageUsers.get_user and ManageRoles.get_role to locate the affected calls and
update load_users and the function definition that has the json parameter.
---
Nitpick comments:
In `@web/setup.py`:
- Around line 246-257: The code assigns auth_source directly from user_entry
into user_data without validating it; update the block that reads auth_source
(the user_entry.get('auth_source', INTERNAL) and the user_data dict) to validate
the value against the allowed constants (INTERNAL, LDAP, OAUTH2, KERBEROS,
WEBSERVER) and normalize casing if desired, and if the provided value is not one
of these, raise or return a clear error before calling create_user so invalid
auth_source values (e.g., "Internal" or typos) are rejected and only recognized
constants are used.
- Around line 209-211: The file opens JSON files using open(file_path) without
an explicit encoding; update the code that reads into jsonlib.load to open the
file with an explicit UTF-8 encoding (i.e., pass encoding="utf-8" to open) so
reads are deterministic across platforms—locate the try block that uses
file_path and jsonlib.load and change the open call accordingly.
- Around line 289-295: Replace the hardcoded minimum password length 6 with the
configured constant by using config.PASSWORD_LENGTH_MIN in the validation and
the error message: in the block that checks auth_source == INTERNAL and inspects
user_data['newPassword'] (and increments error_count/continues), change the
numeric literal to config.PASSWORD_LENGTH_MIN and update the printed string to
interpolate that constant so the message reflects the configured minimum.
| @app.command() | ||
| @update_sqlite_path | ||
| def load_users(input_file: str, | ||
| sqlite_path: Optional[str] = None, | ||
| json: Optional[bool] = False): |
There was a problem hiding this comment.
json parameter is declared but never used — output format is unaffected.
The json parameter (line 170) is accepted but never referenced in the method body, so passing --json has no effect. Either implement JSON output (consistent with other commands like get-users) or remove the parameter. The sqlite_path parameter is consumed by the @update_sqlite_path decorator, so that one is fine.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 169-169: Unused method argument: sqlite_path
(ARG002)
[warning] 170-170: Unused method argument: json
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/setup.py` around lines 166 - 170, The load_users command currently
accepts a json parameter but never uses it; update the load_users function to
honor the json flag (or remove the parameter) — specifically, in the load_users
function implement conditional output: after loading users, if json is True
serialize the resulting users list to JSON (matching the format used by the
get-users command) and print to stdout (or write to the same destination
get-users uses for JSON), otherwise keep the existing human-readable output;
keep the `@update_sqlite_path` decorator as-is and use the existing user-loading
logic in load_users to produce the object you will JSON-serialize.
| try: | ||
| file_path = unquote(input_file) | ||
| except Exception as e: | ||
| print(str(e)) | ||
| return _handle_error(str(e), True) | ||
|
|
||
| # Read and parse JSON file | ||
| try: | ||
| with open(file_path) as f: | ||
| data = jsonlib.load(f) | ||
| except jsonlib.decoder.JSONDecodeError as e: | ||
| return _handle_error( | ||
| gettext("Error parsing input file %s: %s" % (file_path, e)), | ||
| True) | ||
| except Exception as e: | ||
| return _handle_error( | ||
| gettext("Error reading input file %s: [%d] %s" % | ||
| (file_path, e.errno, e.strerror)), True) |
There was a problem hiding this comment.
Double error output on unquote failure (lines 204-206).
When the unquote call fails, str(e) is printed on line 205, then _handle_error(str(e), True) (line 206) prints the same message again before calling sys.exit(1). The JSON-parsing error paths (lines 212-219) correctly delegate to _handle_error only. Remove the extra print on line 205.
Proposed fix
try:
file_path = unquote(input_file)
except Exception as e:
- print(str(e))
return _handle_error(str(e), True)📝 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.
| try: | |
| file_path = unquote(input_file) | |
| except Exception as e: | |
| print(str(e)) | |
| return _handle_error(str(e), True) | |
| # Read and parse JSON file | |
| try: | |
| with open(file_path) as f: | |
| data = jsonlib.load(f) | |
| except jsonlib.decoder.JSONDecodeError as e: | |
| return _handle_error( | |
| gettext("Error parsing input file %s: %s" % (file_path, e)), | |
| True) | |
| except Exception as e: | |
| return _handle_error( | |
| gettext("Error reading input file %s: [%d] %s" % | |
| (file_path, e.errno, e.strerror)), True) | |
| try: | |
| file_path = unquote(input_file) | |
| except Exception as e: | |
| return _handle_error(str(e), True) | |
| # Read and parse JSON file | |
| try: | |
| with open(file_path) as f: | |
| data = jsonlib.load(f) | |
| except jsonlib.decoder.JSONDecodeError as e: | |
| return _handle_error( | |
| gettext("Error parsing input file %s: %s" % (file_path, e)), | |
| True) | |
| except Exception as e: | |
| return _handle_error( | |
| gettext("Error reading input file %s: [%d] %s" % | |
| (file_path, e.errno, e.strerror)), True) |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 204-204: Do not catch blind exception: Exception
(BLE001)
[warning] 216-216: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/setup.py` around lines 202 - 219, Remove the redundant print of the
exception in the unquote error path: when unquote(input_file) raises, stop
calling print(str(e)) and instead directly return _handle_error(str(e), True)
(same behavior as the JSON error branches). Update the try/except around unquote
to catch Exception as e, assign file_path = unquote(input_file) in the try, and
in the except simply return _handle_error(str(e), True) (references: unquote,
input_file, file_path, _handle_error).
| if 'username' not in user_entry and\ | ||
| 'email' not in user_entry: | ||
| print(f"Skipping user: missing 'username' or 'email'") | ||
| error_count += 1 | ||
| continue |
There was a problem hiding this comment.
f-string on line 242 has no placeholders — include the entry context for debuggability.
Static analysis (Ruff F541) flags this. More importantly, when a user entry lacks both username and email, the current message doesn't identify which entry in the array caused the issue, making debugging hard for large imports.
Proposed fix
- if 'username' not in user_entry and\
- 'email' not in user_entry:
- print(f"Skipping user: missing 'username' or 'email'")
+ if 'username' not in user_entry and\
+ 'email' not in user_entry:
+ print("Skipping user entry: missing 'username' "
+ "or 'email'")🧰 Tools
🪛 Ruff (0.15.1)
[error] 242-242: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/setup.py` around lines 240 - 244, The print statement in the user import
block (where user_entry is validated) uses an f-string with no placeholders and
doesn't identify which entry failed; update the message to include context
(e.g., the user_entry contents or its index) so failures are debuggable and to
satisfy Ruff F541. Locate the validation around the user_entry variable in
web/setup.py (the block that checks 'username' and 'email') and change the print
to include either the current index from an enumerate or the user_entry dict (or
both) in the output, and keep incrementing error_count and continue as before.
| uid = ManageUsers.get_user( | ||
| username=user_data['username'], | ||
| auth_source=auth_source) | ||
| if uid: | ||
| print(f"Skipping user '{user_data['username']}': " | ||
| f"already exists") | ||
| skipped_count += 1 | ||
| continue | ||
|
|
||
| # Get role ID | ||
| rid = ManageRoles.get_role(user_data['role']) | ||
| if rid is None: | ||
| print(f"Skipping user '{user_data['username']}': " | ||
| f"role '{user_data['role']}' does not exist") | ||
| error_count += 1 | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "def get_user" | head -5Repository: pgadmin-org/pgadmin4
Length of output: 452
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "def get_role" | head -5Repository: pgadmin-org/pgadmin4
Length of output: 143
🏁 Script executed:
cat -n web/setup.py | sed -n '200,315p'Repository: pgadmin-org/pgadmin4
Length of output: 5715
🏁 Script executed:
cat -n web/setup.py | grep -A 20 "def get_user"Repository: pgadmin-org/pgadmin4
Length of output: 3337
🏁 Script executed:
cat -n web/setup.py | grep -A 20 "def get_role"Repository: pgadmin-org/pgadmin4
Length of output: 763
🏁 Script executed:
# Check the json parameter usage
cat -n web/setup.py | sed -n '166,200p'Repository: pgadmin-org/pgadmin4
Length of output: 1395
🏁 Script executed:
cat -n web/setup.py | sed -n '166,320p' | grep -E "(json|_handle_error)"Repository: pgadmin-org/pgadmin4
Length of output: 444
🏁 Script executed:
# Find _handle_error definition
cat -n web/setup.py | grep -A 10 "def _handle_error"Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Search for _handle_error definition more broadly
rg "def _handle_error" web/setup.py -A 5Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Check if json parameter is actually used anywhere in load_users
rg "json" web/setup.py | grep -i "load_users\|def load_users" -A 30Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Search for _handle_error in the entire file
grep -n "_handle_error" web/setup.py | head -10Repository: pgadmin-org/pgadmin4
Length of output: 490
🏁 Script executed:
# Look for the definition of _handle_error
grep -B 2 -A 8 "_handle_error" web/setup.py | head -40Repository: pgadmin-org/pgadmin4
Length of output: 1620
🏁 Script executed:
# Check how json parameter is used in the entire ManageUsers class
sed -n '166,320p' web/setup.py | cat -nRepository: pgadmin-org/pgadmin4
Length of output: 7212
🏁 Script executed:
# Find the INTERNAL, LDAP etc. constants
rg "INTERNAL\s*=" web/pgadmin/utils/constants.pyRepository: pgadmin-org/pgadmin4
Length of output: 85
🏁 Script executed:
# Also check what _handle_error does
rg "def _handle_error" web/pgadmin -A 8Repository: pgadmin-org/pgadmin4
Length of output: 551
Creating a new Flask app per user inside the loop is very expensive.
ManageUsers.get_user() (line 270) and ManageRoles.get_role() (line 280) each internally call create_app() and open their own test_request_context. Since load_users already has an app context at line 236, these calls create 2×N redundant Flask application instances for N users. This will make bulk import extremely slow.
Query the User and Role models directly within the existing app context instead.
Additionally:
- Line 242: f-string without placeholders (
f"Skipping user: missing...") should be a regular string - Line 291: hardcoded
6should useconfig.PASSWORD_LENGTH_MIN - Line 210:
open(file_path)should specify encoding - Lines 205–206: error is printed twice (once explicitly, then by
_handle_error) jsonparameter (line 170) is unused
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/setup.py` around lines 270 - 285, Replace calls to ManageUsers.get_user
and ManageRoles.get_role inside load_users with direct ORM queries against the
User and Role models using the existing app context (avoid calling
create_app()/test_request_context inside ManageUsers/ManageRoles to eliminate
per-user app creation); also change the f-string "f\"Skipping user:
missing...\"" to a plain string, replace the hardcoded password min length 6
with config.PASSWORD_LENGTH_MIN, open files with explicit encoding (e.g.
open(file_path, encoding="utf-8")), remove the redundant explicit print of
errors so only _handle_error prints/logs them, and remove the unused json
parameter from the function signature and any callers. Ensure you reference
ManageUsers.get_user and ManageRoles.get_role to locate the affected calls and
update load_users and the function definition that has the json parameter.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/utils/session.py (1)
105-105:⚠️ Potential issue | 🟡 MinorPipeline E302: add a second blank line before
class CachingSessionManager.pycodestyle requires two blank lines between top-level definitions; only one is present here.
🐛 Proposed fix
def put(self, session): 'Store a managed session' raise NotImplementedError + class CachingSessionManager(SessionManager):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/utils/session.py` at line 105, Add one more blank line immediately before the declaration of class CachingSessionManager (the class that subclasses SessionManager) so there are two blank lines separating this top-level class definition from the previous top-level code; this will satisfy pycodestyle E302 by ensuring two blank lines before the CachingSessionManager class declaration.
🤖 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/utils/session.py`:
- Line 161: The conditional "if self.is_session_ready(session) and
session.hmac_digest != digest:" in session.py exceeds the 79-char limit; split
it across multiple lines by adding parentheses or temporary booleans to wrap the
two checks (e.g., assign self.is_session_ready(session) to a variable like ready
or place the two expressions on separate lines within parentheses) so the
condition reads across multiple indented lines and stays under 79 chars while
preserving the same logic for is_session_ready, session.hmac_digest and digest.
- Around line 41-42: Remove the standalone import "from flask import
has_request_context" and add has_request_context to the existing "from flask
import ..." import that already brings in other Flask names (the import that
currently imports current_app/g/session or similar), consolidating all Flask
imports into one statement and deleting the duplicate line.
---
Outside diff comments:
In `@web/pgadmin/utils/session.py`:
- Line 105: Add one more blank line immediately before the declaration of class
CachingSessionManager (the class that subclasses SessionManager) so there are
two blank lines separating this top-level class definition from the previous
top-level code; this will satisfy pycodestyle E302 by ensuring two blank lines
before the CachingSessionManager class declaration.
| from flask import has_request_context | ||
|
|
There was a problem hiding this comment.
Consolidate the duplicate flask import into line 29.
has_request_context is imported in a standalone statement while line 29 already imports other Flask names.
♻️ Proposed fix
-from flask import current_app, request, flash, redirect
+from flask import current_app, has_request_context, request, flash, redirectAnd remove line 41:
-from flask import has_request_context
-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/utils/session.py` around lines 41 - 42, Remove the standalone
import "from flask import has_request_context" and add has_request_context to
the existing "from flask import ..." import that already brings in other Flask
names (the import that currently imports current_app/g/session or similar),
consolidating all Flask imports into one statement and deleting the duplicate
line.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/pgadmin/utils/session.py (1)
41-42: Consolidate the duplicateflaskimport into line 29.This was raised in a previous review.
has_request_contextshould be added to the existingfrom flask import ...statement on line 29 instead of being a separate import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/utils/session.py` around lines 41 - 42, Consolidate the duplicate Flask import by removing the separate "from flask import has_request_context" line and adding has_request_context to the existing "from flask import ..." import statement at the top of the file (the current import that brings in other flask symbols), so only one from-flask import remains; ensure no other references are changed.
🤖 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/utils/session.py`:
- Around line 120-129: The is_session_ready method should guard against _session
being None and catch the correct dict-related exceptions from ManagedSession (a
CallbackDict subclass). Change is_session_ready to return False immediately if
_session is None (or check with "if not _session: return False"), then access
the id safely (e.g., use _session.get('_id') or keep the index access but update
the except clause); replace the existing except (AssertionError, RuntimeError)
with an except that catches KeyError (and optionally TypeError for safety) so
missing keys or wrong types no longer crash when ManagedSession or None is
passed.
---
Duplicate comments:
In `@web/pgadmin/utils/session.py`:
- Around line 41-42: Consolidate the duplicate Flask import by removing the
separate "from flask import has_request_context" line and adding
has_request_context to the existing "from flask import ..." import statement at
the top of the file (the current import that brings in other flask symbols), so
only one from-flask import remains; ensure no other references are changed.
| def is_session_ready(self, _session): | ||
| if not has_request_context(): | ||
| return False | ||
|
|
||
| # Session _id returns the str object | ||
| # or None if it hasn't been set yet. | ||
| try: | ||
| return _session['_id'] is not None | ||
| except (AssertionError, RuntimeError): | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ManagedSession class and its inheritance
rg -n 'class ManagedSession' --type=py -A10
# Check CallbackDict (from Flask/Werkzeug)
rg -n 'class CallbackDict' --type=py -A10
# Find all callers of is_session_ready to see what _session can be
rg -n 'is_session_ready' --type=py -B2 -A2
# Check line 169 specifically to see what's passed
sed -n '165,175p' web/pgadmin/utils/session.py
# Check line 164 to understand cache behavior
sed -n '160,170p' web/pgadmin/utils/session.pyRepository: pgadmin-org/pgadmin4
Length of output: 3378
TypeError crash when _session is None; exception types don't match dict access behavior.
The method is called at line 169 where session can be None (set on line 164 when HMAC digest mismatch is detected). Passing None to None['_id'] raises TypeError, which the except clause does not catch—causing a crash during normal operation.
Additionally, ManagedSession extends CallbackDict (a Werkzeug dict subclass). Missing dictionary keys raise KeyError, not AssertionError or RuntimeError. The except clause catches the wrong exception types entirely.
Proposed fix — add None guard and catch the correct exception
def is_session_ready(self, _session):
+ if _session is None:
+ return False
+
if not has_request_context():
return False
# Session _id returns the str object
# or None if it hasn't been set yet.
try:
return _session['_id'] is not None
- except (AssertionError, RuntimeError):
+ except (KeyError, RuntimeError):
return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/utils/session.py` around lines 120 - 129, The is_session_ready
method should guard against _session being None and catch the correct
dict-related exceptions from ManagedSession (a CallbackDict subclass). Change
is_session_ready to return False immediately if _session is None (or check with
"if not _session: return False"), then access the id safely (e.g., use
_session.get('_id') or keep the index access but update the except clause);
replace the existing except (AssertionError, RuntimeError) with an except that
catches KeyError (and optionally TypeError for safety) so missing keys or wrong
types no longer crash when ManagedSession or None is passed.
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes