Conversation
📝 WalkthroughWalkthroughAdds imio.fpaudit integration: new audit module, registry setup for an "esign" audit log, test patching for fplog, and audit() calls across session/file views, actions, and feedback handling; also updates packaging and profile dependencies and changelog. Changes
Sequence Diagram(s)(none — changes are instrumentation-focused and do not introduce a new multi-component control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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)
Comment |
chris-adam
force-pushed
the
PARAF-337/esign_audit
branch
3 times, most recently
from
ca72b7f to
a898756
Compare
chris-adam
commented
Mar 5, 2026
| self.request.response.setStatus(500) | ||
| logger.error(str(e)) | ||
| return {"message": str(e)} | ||
| audit("session_feedback", "session={} code={} db_state={}".format(session_id, code, db_state)) |
Contributor
Author
There was a problem hiding this comment.
A voir si c'est vraiment utile de logger cette action
chris-adam
force-pushed
the
PARAF-337/esign_audit
branch
2 times, most recently
from
1d3b5a0 to
b82bd1e
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: 4
🧹 Nitpick comments (2)
src/imio/esign/setuphandlers.py (1)
39-41: Avoid duplicating the audit log identifier literal.
"esign"is now defined in multiple files (setuphandlers.pyandaudit.py). Centralizing it avoids silent drift between runtime logging and registry setup.Proposed refactor
from imio.esign.config import get_registry_parapheo_url from imio.esign.config import get_registry_signing_users_email_content from imio.esign.config import set_registry_parapheo_url from imio.esign.config import set_registry_signing_users_email_content from imio.esign.config import SIGNERS_EMAIL_CONTENT +from imio.esign.audit import LOG_ID from plone.registry.interfaces import IRegistry from Products.CMFPlone.interfaces import INonInstallable from zope.component import getUtility from zope.interface import implementer @@ - if not any(e.get("log_id") == u"esign" for e in entries): - entries.append({u"log_id": u"esign", u"audit_log": u"esign.log", + if not any(e.get("log_id") == LOG_ID for e in entries): + entries.append({u"log_id": LOG_ID, u"audit_log": u"esign.log", u"log_format": u"%(asctime)s - %(message)s"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/setuphandlers.py` around lines 39 - 41, The code duplicates the "esign" audit log identifier literal across modules; create a single exported constant (e.g., ESIGN_LOG_ID) in the module that defines the audit behavior (audit.py) and import that constant into setuphandlers.py, then replace the literal checks and entries.append usage (the generator expression any(e.get("log_id") == u"esign" and the dict {u"log_id": u"esign", ...}) with references to ESIGN_LOG_ID; search for other occurrences of the literal and replace them to centralize the identifier so runtime logging and registry setup reference the same symbol.setup.py (1)
58-58: Add a minimum compatible version forimio.fpaudit.The dependency is currently unbounded on line 58, whereas similar dependencies like
imio.helpers>1.3.10andplone.api>=1.8.4include version constraints. Since this PR relies on specificimio.fpauditbehavior (logging utility + profile/registry integration), a minimum version constraint should be specified to prevent incompatible older versions from being installed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.py` at line 58, The dependency entry imio.fpaudit in the install_requires list is unbounded; update that string to include a minimum compatible version (e.g. change "imio.fpaudit" to "imio.fpaudit>=1.0.0") so the package resolver won't install older incompatible releases that lack the logging utility and profile/registry integration relied upon by this PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/esign/__init__.py`:
- Around line 19-27: Replace the unreliable ZOPE_HOME presence check with an
explicit test flag: change the condition that currently reads
os.environ.get("ZOPE_HOME") is None to check for
os.environ.get("IMIO_ESIGN_TESTING") == "1" (or similar truthy value) so
patching only occurs in test mode; keep the mocked pieces logged_actions and
mock_fplog and the assignment _fpaudit_utils.fplog = mock_fplog but ensure you
also replace the deprecated logger.warn call with logger.warning to avoid
deprecation warnings and use the new flag to control when the mock is installed.
In `@src/imio/esign/audit.py`:
- Around line 8-10: The audit helper currently lets exceptions from imio.fpaudit
propagate; update the audit(action, extras="") function to wrap the call to
fpaudit_utils.fplog(LOG_ID, action, extras) in a try-except that catches
Exception and logs the failure using the module logger (imio.esign), e.g.,
logger.exception or logger.error with exception info, so audit failures are
swallowed and only logged instead of raising and breaking request handlers that
call audit.
In `@src/imio/esign/browser/actions.py`:
- Around line 100-105: The audit currently logs a single session by taking only
the first file UID; instead, gather all file_uids = annot.get("c_uids",
{}).get(uid, []) and derive session_ids = {annot.get("uids", {}).get(fuid) for
fuid in file_uids if annot.get("uids", {}).get(fuid) is not None}, deduplicate
them, call remove_context_from_session(context_uids=[uid]) as before, and then
call audit("remove_context_from_session", "sessions={}
context={}".format(list(session_ids), uid)) so the audit records all affected
session IDs (reference functions/methods: get_uid_to_remove,
get_session_annotation, remove_context_from_session, audit).
- Around line 125-128: The audit "remove_item_from_session" is emitted even when
nothing was removed because session_id can be None and remove_files_from_session
may no-op; update the action to only emit the audit when a removal actually
occurred: call get_session_annotation()/context.UID() as before, then call
remove_files_from_session and capture its result (or check whether session_id is
truthy), and only call audit("remove_item_from_session", ...) if the removal
function indicates success or if session_id is not None/empty. Reference the
symbols context.UID(), get_session_annotation(), remove_files_from_session, and
audit to locate and gate the audit emission.
---
Nitpick comments:
In `@setup.py`:
- Line 58: The dependency entry imio.fpaudit in the install_requires list is
unbounded; update that string to include a minimum compatible version (e.g.
change "imio.fpaudit" to "imio.fpaudit>=1.0.0") so the package resolver won't
install older incompatible releases that lack the logging utility and
profile/registry integration relied upon by this PR.
In `@src/imio/esign/setuphandlers.py`:
- Around line 39-41: The code duplicates the "esign" audit log identifier
literal across modules; create a single exported constant (e.g., ESIGN_LOG_ID)
in the module that defines the audit behavior (audit.py) and import that
constant into setuphandlers.py, then replace the literal checks and
entries.append usage (the generator expression any(e.get("log_id") == u"esign"
and the dict {u"log_id": u"esign", ...}) with references to ESIGN_LOG_ID; search
for other occurrences of the literal and replace them to centralize the
identifier so runtime logging and registry setup reference the same symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ab3c1ab-be83-438b-98c2-006dc0dd4b52
📒 Files selected for processing (10)
CHANGES.rstsetup.pysrc/imio/esign/__init__.pysrc/imio/esign/audit.pysrc/imio/esign/browser/actions.pysrc/imio/esign/browser/views.pysrc/imio/esign/configure.zcmlsrc/imio/esign/profiles/default/metadata.xmlsrc/imio/esign/services/external_session_feedback.pysrc/imio/esign/setuphandlers.py
Comment on lines
+19
to
+27
| if os.environ.get("ZOPE_HOME") is None: # test env | ||
| logged_actions = [] | ||
|
|
||
| def mock_fplog(log_id, action, extras): | ||
| logged_actions.append((log_id, action, extras)) | ||
|
|
||
| logger.warn("PATCHING imio.fpaudit.utils.fplog") | ||
| _fpaudit_utils.fplog = mock_fplog | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=py 'ZOPE_HOME|IMIO_ESIGN_TESTING|logger\.warn\(|logger\.warning\(' -A 2 -B 2Repository: IMIO/imio.esign
Length of output: 640
🏁 Script executed:
# Check if ZOPE_HOME is typically set in this project and how testing is usually configured
find . -name "*.cfg" -o -name "*.ini" -o -name "*.yaml" -o -name "*.yml" -o -name "conftest.py" -o -name "tox.ini" | head -20Repository: IMIO/imio.esign
Length of output: 197
🏁 Script executed:
# Look at the full context of the problematic code
head -40 src/imio/esign/__init__.pyRepository: IMIO/imio.esign
Length of output: 1074
🏁 Script executed:
cat base.cfg | grep -i "zope_home" -A 2 -B 2Repository: IMIO/imio.esign
Length of output: 41
🏁 Script executed:
cat test-4.3.cfg | grep -i "zope_home" -A 2 -B 2Repository: IMIO/imio.esign
Length of output: 41
🏁 Script executed:
cat test-6.1.cfg | grep -i "zope_home" -A 2 -B 2Repository: IMIO/imio.esign
Length of output: 41
🏁 Script executed:
# Check GitHub Actions workflow to see how tests are run
cat .github/workflows/main.yml | head -80Repository: IMIO/imio.esign
Length of output: 2433
🏁 Script executed:
cat base.cfgRepository: IMIO/imio.esign
Length of output: 2045
🏁 Script executed:
cat gha.cfgRepository: IMIO/imio.esign
Length of output: 175
🏁 Script executed:
# Research Python logger.warn() deprecation
python3 <<'EOF'
import logging
import sys
# Check if warn is deprecated
logger = logging.getLogger("test")
# Check if both methods exist and their status
if hasattr(logger, 'warn'):
print("logger.warn() exists")
if hasattr(logger, 'warning'):
print("logger.warning() exists")
# Check deprecation warnings in Python docs
print("\nPython logging module info:")
print(f"Python version: {sys.version}")
# Try to check the source or documentation
import inspect
try:
warn_source = inspect.getsource(logger.warn)
print("\nlogger.warn() is still available in source")
except:
print("\nlogger.warn() not available")
try:
warning_source = inspect.getsource(logger.warning)
print("logger.warning() is available in source")
except:
print("logger.warning() not available")
EOFRepository: IMIO/imio.esign
Length of output: 282
🏁 Script executed:
# Check if there are any test files that import or test the fplog mocking
find . -name "test*.py" -o -name "*test.py" | head -10Repository: IMIO/imio.esign
Length of output: 244
🏁 Script executed:
cat src/imio/esign/testing.pyRepository: IMIO/imio.esign
Length of output: 1761
🏁 Script executed:
# Check test files for ZOPE_HOME references
rg "ZOPE_HOME" src/imio/esign/tests/Repository: IMIO/imio.esign
Length of output: 41
🏁 Script executed:
# Research Python logging deprecation in official documentation
python3 <<'EOF'
import logging
import warnings
# Check docstring for warn method
logger = logging.getLogger("test")
print("logger.warn() docstring:")
print(logger.warn.__doc__)
print("\n" + "="*60 + "\n")
print("logger.warning() docstring:")
print(logger.warning.__doc__)
# Check if warn is an alias
print("\n" + "="*60 + "\n")
print(f"logger.warn is logger.warning: {logger.warn is logger.warning}")
EOFRepository: IMIO/imio.esign
Length of output: 526
Use an explicit test environment flag instead of checking for missing ZOPE_HOME.
The current condition os.environ.get("ZOPE_HOME") is None is an unreliable test detector. In production Plone installations, ZOPE_HOME is set; however, any minimal deployment or non-standard environment without this variable will incorrectly trigger the test patch and disable real auditing. Use an explicit flag like IMIO_ESIGN_TESTING="1" to control patching. Also replace deprecated logger.warn() with logger.warning().
Suggested fix
-if os.environ.get("ZOPE_HOME") is None: # test env
+if os.environ.get("IMIO_ESIGN_TESTING") == "1":
logged_actions = []
def mock_fplog(log_id, action, extras):
logged_actions.append((log_id, action, extras))
- logger.warn("PATCHING imio.fpaudit.utils.fplog")
+ logger.warning("Patching imio.fpaudit.utils.fplog for tests")
_fpaudit_utils.fplog = mock_fplog📝 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.
Suggested change
| if os.environ.get("ZOPE_HOME") is None: # test env | |
| logged_actions = [] | |
| def mock_fplog(log_id, action, extras): | |
| logged_actions.append((log_id, action, extras)) | |
| logger.warn("PATCHING imio.fpaudit.utils.fplog") | |
| _fpaudit_utils.fplog = mock_fplog | |
| if os.environ.get("IMIO_ESIGN_TESTING") == "1": | |
| logged_actions = [] | |
| def mock_fplog(log_id, action, extras): | |
| logged_actions.append((log_id, action, extras)) | |
| logger.warning("Patching imio.fpaudit.utils.fplog for tests") | |
| _fpaudit_utils.fplog = mock_fplog |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/__init__.py` around lines 19 - 27, Replace the unreliable
ZOPE_HOME presence check with an explicit test flag: change the condition that
currently reads os.environ.get("ZOPE_HOME") is None to check for
os.environ.get("IMIO_ESIGN_TESTING") == "1" (or similar truthy value) so
patching only occurs in test mode; keep the mocked pieces logged_actions and
mock_fplog and the assignment _fpaudit_utils.fplog = mock_fplog but ensure you
also replace the deprecated logger.warn call with logger.warning to avoid
deprecation warnings and use the new flag to control when the mock is installed.
Comment on lines
+8
to
+10
| def audit(action, extras=""): | ||
| """Log an eSignature action to the dedicated audit log.""" | ||
| fpaudit_utils.fplog(LOG_ID, action, extras) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Step 1: Find all calls to audit() function (excluding the definition)
echo "=== Calls to audit() function ==="
rg -n --type=py '\baudit\(' src/ | grep -v 'def audit'
# Step 2: Check if the file exists and show its structure
echo -e "\n=== Audit.py file structure ==="
if [ -f "src/imio/esign/audit.py" ]; then
wc -l src/imio/esign/audit.py
cat -n src/imio/esign/audit.py
fiRepository: IMIO/imio.esign
Length of output: 1886
🏁 Script executed:
#!/bin/bash
# Step 1: Look at the call context in views.py to confirm request handler context
echo "=== Views.py call contexts ==="
sed -n '125,190p' src/imio/esign/browser/views.py
# Step 2: Look at actions.py context
echo -e "\n=== Actions.py call contexts ==="
sed -n '40,110p' src/imio/esign/browser/actions.py
# Step 3: Check setup.py or requirements to understand fpaudit dependency
echo -e "\n=== Dependency info ==="
if [ -f "setup.py" ]; then
grep -i "fpaudit" setup.py
elif [ -f "setup.cfg" ]; then
grep -i "fpaudit" setup.cfg
elif [ -f "requirements.txt" ]; then
grep -i "fpaudit" requirements.txt
elif [ -f "pyproject.toml" ]; then
grep -i "fpaudit" pyproject.toml
fiRepository: IMIO/imio.esign
Length of output: 5924
🏁 Script executed:
#!/bin/bash
# Step 1: Check if fpaudit is available in the repo or if we need to understand it from context
echo "=== Looking for fpaudit imports and usage patterns ==="
rg -n "from imio.fpaudit" src/
echo -e "\n=== Check for similar try-except patterns for external service calls ==="
rg -n "try:" src/imio/esign --type=py -A 3 | head -50
echo -e "\n=== Look for logging patterns in the codebase ==="
rg -n "logging.getLogger" src/imio/esign --type=py
echo -e "\n=== Check if there are any exception handlers that suppress exceptions ==="
rg -n "except.*:" src/imio/esign --type=py -A 2 | head -60Repository: IMIO/imio.esign
Length of output: 3750
🏁 Script executed:
#!/bin/bash
# Step 1: Check the __init__.py file for logger and fpaudit setup
echo "=== __init__.py content ==="
cat -n src/imio/esign/__init__.py
# Step 2: Look for any documentation or comments about fpaudit exceptions
echo -e "\n=== Search for error handling related to audit ==="
rg -n "fplog\|fpaudit" src/imio/esign --type=py -B 2 -A 2
# Step 3: Check if there are any existing issues or TODOs mentioning audit
echo -e "\n=== Search for audit-related comments ==="
rg -n "audit" src/imio/esign --type=py | grep -i "todo\|fixme\|bug\|exception\|error"Repository: IMIO/imio.esign
Length of output: 1952
Prevent audit backend failures from breaking business flows.
audit() currently propagates exceptions from imio.fpaudit. Since this helper is used in request handlers (11 call sites across views.py, actions.py, and external_session_feedback.py), an audit logging failure can turn successful operations into 500 errors.
Wrap fpaudit_utils.fplog() in a try-except block that catches all exceptions and logs them instead. The codebase already has a logger available (imio.esign), and similar error handling patterns exist elsewhere (e.g., external_session_feedback.py).
Proposed hardening
# -*- coding: utf-8 -*-
from imio.fpaudit import utils as fpaudit_utils
+import logging
@@
LOG_ID = "esign"
+logger = logging.getLogger("imio.esign")
@@
def audit(action, extras=""):
"""Log an eSignature action to the dedicated audit log."""
- fpaudit_utils.fplog(LOG_ID, action, extras)
+ try:
+ fpaudit_utils.fplog(LOG_ID, action, extras)
+ except Exception:
+ logger.exception("Audit logging failed for action=%s", action)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/audit.py` around lines 8 - 10, The audit helper currently lets
exceptions from imio.fpaudit propagate; update the audit(action, extras="")
function to wrap the call to fpaudit_utils.fplog(LOG_ID, action, extras) in a
try-except that catches Exception and logs the failure using the module logger
(imio.esign), e.g., logger.exception or logger.error with exception info, so
audit failures are swallowed and only logged instead of raising and breaking
request handlers that call audit.
Comment on lines
+100
to
+105
| uid = self.get_uid_to_remove() | ||
| annot = get_session_annotation() | ||
| file_uids = annot.get("c_uids", {}).get(uid, []) | ||
| session_id = annot.get("uids", {}).get(file_uids[0]) if file_uids else None | ||
| remove_context_from_session(context_uids=[uid]) | ||
| audit("remove_context_from_session", "session={} context={}".format(session_id, uid)) |
There was a problem hiding this comment.
Audit entry can record the wrong session for context removal.
Line 103 derives session_id from only the first file UID, but Line 104 removes all files tied to the context. This can misattribute the operation in audit logs.
Suggested fix
uid = self.get_uid_to_remove()
annot = get_session_annotation()
- file_uids = annot.get("c_uids", {}).get(uid, [])
- session_id = annot.get("uids", {}).get(file_uids[0]) if file_uids else None
+ file_uids = list(annot.get("c_uids", {}).get(uid, []))
+ uids_map = annot.get("uids", {})
+ session_ids = sorted({uids_map.get(f_uid) for f_uid in file_uids if uids_map.get(f_uid) is not None})
remove_context_from_session(context_uids=[uid])
- audit("remove_context_from_session", "session={} context={}".format(session_id, uid))
+ if not file_uids:
+ audit("remove_context_from_session", "context={} error=context_not_linked".format(uid))
+ else:
+ audit(
+ "remove_context_from_session",
+ "sessions={} context={} files={}".format(",".join(map(str, session_ids)), uid, len(file_uids)),
+ )
self._finished()📝 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.
Suggested change
| uid = self.get_uid_to_remove() | |
| annot = get_session_annotation() | |
| file_uids = annot.get("c_uids", {}).get(uid, []) | |
| session_id = annot.get("uids", {}).get(file_uids[0]) if file_uids else None | |
| remove_context_from_session(context_uids=[uid]) | |
| audit("remove_context_from_session", "session={} context={}".format(session_id, uid)) | |
| uid = self.get_uid_to_remove() | |
| annot = get_session_annotation() | |
| file_uids = list(annot.get("c_uids", {}).get(uid, [])) | |
| uids_map = annot.get("uids", {}) | |
| session_ids = sorted({uids_map.get(f_uid) for f_uid in file_uids if uids_map.get(f_uid) is not None}) | |
| remove_context_from_session(context_uids=[uid]) | |
| if not file_uids: | |
| audit("remove_context_from_session", "context={} error=context_not_linked".format(uid)) | |
| else: | |
| audit( | |
| "remove_context_from_session", | |
| "sessions={} context={} files={}".format(",".join(map(str, session_ids)), uid, len(file_uids)), | |
| ) | |
| self._finished() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/browser/actions.py` around lines 100 - 105, The audit
currently logs a single session by taking only the first file UID; instead,
gather all file_uids = annot.get("c_uids", {}).get(uid, []) and derive
session_ids = {annot.get("uids", {}).get(fuid) for fuid in file_uids if
annot.get("uids", {}).get(fuid) is not None}, deduplicate them, call
remove_context_from_session(context_uids=[uid]) as before, and then call
audit("remove_context_from_session", "sessions={}
context={}".format(list(session_ids), uid)) so the audit records all affected
session IDs (reference functions/methods: get_uid_to_remove,
get_session_annotation, remove_context_from_session, audit).
Contributor
Author
There was a problem hiding this comment.
I added a ticket for that, it will be handled in another pR
There was a problem hiding this comment.
@chris-adam, understood — thanks for the heads-up! Since you've already created a ticket, no further action is needed here. Feel free to link it in this PR for traceability if you'd like.
Comment on lines
+125
to
+128
| uid = self.context.UID() | ||
| session_id = get_session_annotation().get("uids", {}).get(uid) | ||
| remove_files_from_session(files_uids=[uid]) | ||
| audit("remove_item_from_session", "session={} file={}".format(session_id, uid)) |
There was a problem hiding this comment.
Removal audit is emitted even when nothing is removed.
At Line 126, session_id can be None; Line 127 then no-ops with an internal error log, but Line 128 still emits a removal audit entry. This creates false positives in the audit trail.
Suggested fix
uid = self.context.UID()
- session_id = get_session_annotation().get("uids", {}).get(uid)
- remove_files_from_session(files_uids=[uid])
- audit("remove_item_from_session", "session={} file={}".format(session_id, uid))
+ session_id = get_session_annotation().get("uids", {}).get(uid)
+ if session_id is None:
+ audit("remove_item_from_session", "file={} error=file_not_linked".format(uid))
+ else:
+ remove_files_from_session(files_uids=[uid])
+ audit("remove_item_from_session", "session={} file={} status=removed".format(session_id, uid))
self._finished()📝 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.
Suggested change
| uid = self.context.UID() | |
| session_id = get_session_annotation().get("uids", {}).get(uid) | |
| remove_files_from_session(files_uids=[uid]) | |
| audit("remove_item_from_session", "session={} file={}".format(session_id, uid)) | |
| uid = self.context.UID() | |
| session_id = get_session_annotation().get("uids", {}).get(uid) | |
| if session_id is None: | |
| audit("remove_item_from_session", "file={} error=file_not_linked".format(uid)) | |
| else: | |
| remove_files_from_session(files_uids=[uid]) | |
| audit("remove_item_from_session", "session={} file={} status=removed".format(session_id, uid)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/browser/actions.py` around lines 125 - 128, The audit
"remove_item_from_session" is emitted even when nothing was removed because
session_id can be None and remove_files_from_session may no-op; update the
action to only emit the audit when a removal actually occurred: call
get_session_annotation()/context.UID() as before, then call
remove_files_from_session and capture its result (or check whether session_id is
truthy), and only call audit("remove_item_from_session", ...) if the removal
function indicates success or if session_id is not None/empty. Reference the
symbols context.UID(), get_session_annotation(), remove_files_from_session, and
audit to locate and gate the audit emission.
b82bd1e to
902acf5
Compare
chris-adam
commented
Mar 5, 2026
|
|
||
| def audit(action, extras=""): | ||
| """Log an eSignature action to the dedicated audit log.""" | ||
| fpaudit_utils.fplog(LOG_ID, action, extras) |
Contributor
Author
There was a problem hiding this comment.
Ici, j'aurais aimé faire comme dans collective.contact.core audit_access et avoir un format consistent avec les mêmes keys systématiquement mais il y a trop de cas différents
Pull Request Test Coverage Report for Build 23446241523Details
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/imio/esign/browser/actions.py (2)
130-133:⚠️ Potential issue | 🟠 MajorRemoval audit is still emitted even when no removal occurs.
If the UID is no longer linked, the operation can no-op while still writing a removal audit entry. This matches the earlier unresolved finding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/actions.py` around lines 130 - 133, The audit "remove_item_from_session" is emitted even when no removal happens; update the logic in the block using self.context.UID(), get_session_annotation(), remove_files_from_session, and audit so you first check whether the session mapping actually contains the uid (e.g., session_annotation.get("uids", {}) contains uid) and only call remove_files_from_session(...) and audit(...) when the uid is present and removal will occur; if the uid is not linked, skip both removal and the audit.
100-105:⚠️ Potential issue | 🟠 MajorContext-removal audit can still misattribute session scope.
This still derives
session_idfrom only the first linked file, so the audit entry may be inaccurate when a context spans multiple sessions. This was already reported earlier and remains unresolved in this diff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/actions.py` around lines 100 - 105, The audit currently computes session_id from only the first file UID (in the block using get_uid_to_remove, get_session_annotation, and file_uids) which can misattribute when a context spans multiple sessions; change the logic to collect all session IDs mapped from file_uids (e.g., derive session_ids = [annot.get("uids", {}).get(f) for f in file_uids if annot.get("uids", {}).get(f) is not None]), handle empty or multiple values, and pass a clear representation (list or joined string) to audit("remove_context_from_session", ...) instead of using only file_uids[0]; keep remove_context_from_session(context_uids=[uid]) unchanged.
🧹 Nitpick comments (1)
src/imio/esign/browser/actions.py (1)
44-45: Remove stale commented audit code.Keeping commented-out statements here adds noise and suggests incomplete behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/actions.py` around lines 44 - 45, Remove the stale commented audit statement that calls audit("add_to_session", ...) — specifically delete the commented lines referencing session_id, self.context.UID(), files_uids and signers in the actions.py block so the dead code is removed and the codebase stays clean; if audit logging is required later, reintroduce it as an active, properly implemented call inside the relevant method (e.g., where session_id and files_uids are available).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/esign/browser/actions.py`:
- Line 36: The assignment from add_files_to_session currently unpacks into
session_id and _session but session_id is unused and triggers RUF059; update the
unpack target in the call to add_files_to_session to use a silent name (e.g.,
_session_id or _ ) instead of session_id so only the intended _session variable
remains used (locate the call to add_files_to_session in this file and rename
session_id accordingly).
In `@src/imio/esign/utils.py`:
- Around line 84-87: The audit call is currently including signer emails via
session_id and signers (the "|".join([sg[1] for sg in signers])) which leaks
PII; change the payload to use non-PII metadata instead (e.g., signer count or
internal signer IDs). Update the audit invocation in the create_session block to
replace the email join with either str(len(signers)) or a join of an internal
identifier field (e.g., sg[0]) while leaving session_id as-is, so the audit
record contains only non-PII signer metadata.
---
Duplicate comments:
In `@src/imio/esign/browser/actions.py`:
- Around line 130-133: The audit "remove_item_from_session" is emitted even when
no removal happens; update the logic in the block using self.context.UID(),
get_session_annotation(), remove_files_from_session, and audit so you first
check whether the session mapping actually contains the uid (e.g.,
session_annotation.get("uids", {}) contains uid) and only call
remove_files_from_session(...) and audit(...) when the uid is present and
removal will occur; if the uid is not linked, skip both removal and the audit.
- Around line 100-105: The audit currently computes session_id from only the
first file UID (in the block using get_uid_to_remove, get_session_annotation,
and file_uids) which can misattribute when a context spans multiple sessions;
change the logic to collect all session IDs mapped from file_uids (e.g., derive
session_ids = [annot.get("uids", {}).get(f) for f in file_uids if
annot.get("uids", {}).get(f) is not None]), handle empty or multiple values, and
pass a clear representation (list or joined string) to
audit("remove_context_from_session", ...) instead of using only file_uids[0];
keep remove_context_from_session(context_uids=[uid]) unchanged.
---
Nitpick comments:
In `@src/imio/esign/browser/actions.py`:
- Around line 44-45: Remove the stale commented audit statement that calls
audit("add_to_session", ...) — specifically delete the commented lines
referencing session_id, self.context.UID(), files_uids and signers in the
actions.py block so the dead code is removed and the codebase stays clean; if
audit logging is required later, reintroduce it as an active, properly
implemented call inside the relevant method (e.g., where session_id and
files_uids are available).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dda0451-7565-44f3-a37d-5f29c35f56a1
📒 Files selected for processing (10)
CHANGES.rstcheckouts.cfgsetup.pysrc/imio/esign/audit.pysrc/imio/esign/browser/actions.pysrc/imio/esign/browser/views.pysrc/imio/esign/configure.zcmlsrc/imio/esign/services/external_session_feedback.pysrc/imio/esign/utils.pytest-4.3.cfg
✅ Files skipped from review due to trivial changes (7)
- setup.py
- src/imio/esign/configure.zcml
- test-4.3.cfg
- src/imio/esign/services/external_session_feedback.py
- CHANGES.rst
- src/imio/esign/audit.py
- checkouts.cfg
🚧 Files skipped from review as they are similar to previous changes (1)
- src/imio/esign/browser/views.py
| return self._finished(failed_msgid="Could not get signers to add to the session!") | ||
| # watchers = self.get_watchers() | ||
| add_files_to_session( | ||
| session_id, _session = add_files_to_session( |
There was a problem hiding this comment.
Silence unused unpacked variable to keep lint clean.
session_id is assigned but not used. Rename it to _session_id (or _) to resolve RUF059.
Proposed fix
- session_id, _session = add_files_to_session(
+ _session_id, _session = add_files_to_session(📝 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.
Suggested change
| session_id, _session = add_files_to_session( | |
| _session_id, _session = add_files_to_session( |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 36-36: Unpacked variable session_id is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/browser/actions.py` at line 36, The assignment from
add_files_to_session currently unpacks into session_id and _session but
session_id is unused and triggers RUF059; update the unpack target in the call
to add_files_to_session to use a silent name (e.g., _session_id or _ ) instead
of session_id so only the intended _session variable remains used (locate the
call to add_files_to_session in this file and rename session_id accordingly).
Comment on lines
+84
to
+87
| audit( | ||
| "create_session", | ||
| "session={} signers={}".format(session_id, "|".join([sg[1] for sg in signers])) | ||
| ) |
There was a problem hiding this comment.
Avoid logging signer emails in audit payloads.
This stores personal data in audit logs; prefer non-PII metadata (e.g., signer count or internal IDs only).
Proposed minimal change
audit(
"create_session",
- "session={} signers={}".format(session_id, "|".join([sg[1] for sg in signers]))
+ "session={} signer_count={}".format(session_id, len(signers))
)📝 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.
Suggested change
| audit( | |
| "create_session", | |
| "session={} signers={}".format(session_id, "|".join([sg[1] for sg in signers])) | |
| ) | |
| audit( | |
| "create_session", | |
| "session={} signer_count={}".format(session_id, len(signers)) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/utils.py` around lines 84 - 87, The audit call is currently
including signer emails via session_id and signers (the "|".join([sg[1] for sg
in signers])) which leaks PII; change the payload to use non-PII metadata
instead (e.g., signer count or internal signer IDs). Update the audit invocation
in the create_session block to replace the email join with either
str(len(signers)) or a join of an internal identifier field (e.g., sg[0]) while
leaving session_id as-is, so the audit record contains only non-PII signer
metadata.
sgeulette
approved these changes
Mar 23, 2026
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
3 participants
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 by CodeRabbit
New Features
Documentation
Chores