codeant-ai-for-open-source[bot] commented on code in PR #42306:
URL: https://github.com/apache/superset/pull/42306#discussion_r3642875923
##########
superset/utils/log.py:
##########
@@ -193,11 +194,14 @@ def log_with_context( # pylint:
disable=too-many-locals,too-many-arguments
if user_id is None and has_request_context():
try:
actual_user = g.get("user", None)
- if actual_user is not None:
+ # Guest/anonymous users (e.g. embedded dashboards) are never
+ # DB-mapped, so adding them to the session always fails.
+ # This is expected and not worth logging.
+ if actual_user is not None and sa_inspect(actual_user,
raiseerr=False):
db.session.add(actual_user)
user_id = get_user_id()
Review Comment:
**Suggestion:** This mapped-instance check is performed against `g.user`
directly, but in Flask-AppBuilder `g.user` is often a `LocalProxy`; SQLAlchemy
inspection on the proxy can return not-mapped even when the underlying
authenticated user model is mapped. That causes the rebind path to be skipped,
so detached real users can still end up with missing `user_id` in logs. Unwrap
the proxy to the underlying user object before calling SQLAlchemy inspection
and adding to the session. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- WARNING Event logging loses user_id in detached-user scenarios.
- WARNING Audit trails missing user information for affected events.
- WARNING Harder to debug issues tied to specific users.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR code and authenticate a real user so that a
request runs
with `has_request_context()` true and `g.user` populated (Flask-AppBuilder
before-request
hook; log_with_context in `superset/utils/log.py` around lines 189–205).
2. Arrange for `get_user_id()` (in `superset/utils/core.py`, called at
`superset/utils/log.py:190`) to return `None` even though the underlying
authenticated
user model is a valid, mapped SQLAlchemy instance (for example, by using a
detached user
instance in `g.user`).
3. Trigger any path that calls `AbstractEventLogger.log_with_context()` so
execution
reaches the block at `superset/utils/log.py:194-205` where `user_id is None
and
has_request_context()` is true and `actual_user = g.get("user", None)` is a
`werkzeug.local.LocalProxy` wrapping the real user.
4. At `superset/utils/log.py:200`, `sa_inspect(actual_user, raiseerr=False)`
is applied to
the `LocalProxy` instead of the underlying user; SQLAlchemy inspection
treats the proxy
type as unmapped, the `if` condition is false, `db.session.add(actual_user)`
is skipped,
and `user_id` remains `None`, so the resulting event log entry has no user
ID even though
a real mapped user was present.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=350208a81d144e88a828f3da568a6e7f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=350208a81d144e88a828f3da568a6e7f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/log.py
**Line:** 200:202
**Comment:**
*Logic Error: This mapped-instance check is performed against `g.user`
directly, but in Flask-AppBuilder `g.user` is often a `LocalProxy`; SQLAlchemy
inspection on the proxy can return not-mapped even when the underlying
authenticated user model is mapped. That causes the rebind path to be skipped,
so detached real users can still end up with missing `user_id` in logs. Unwrap
the proxy to the underlying user object before calling SQLAlchemy inspection
and adding to the session.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42306&comment_hash=6350e80a5da1227a6ff9432f8c5db2e7edad138c7ffa0be2e919f5c02ed5be90&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42306&comment_hash=6350e80a5da1227a6ff9432f8c5db2e7edad138c7ffa0be2e919f5c02ed5be90&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
