eschutho opened a new pull request, #44213:
URL: https://github.com/apache/superset/pull/44213
### SUMMARY
`CurrentUserRestApi`'s three `/api/v1/me/` endpoints — `get_me`,
`get_my_roles`, and `update_me` — resolve `g.user` (a flask-login lazy proxy)
inside their method bodies. When flask-login lazily resolves that proxy for a
**validly-authenticated** JWT whose bearer is a real workspace member with **no
assigned role** (Preset Manager's "no access" sentinel), it raises
`UserLookupError("No access")`. A missing/invalid authorization context
similarly raises `NoAuthorizationError`.
Neither exception was caught anywhere in the call chain. It propagated all
the way to flask_appbuilder's `@safe` decorator, whose bare `except Exception:
log.exception(e); return self.response_500(...)` logs a **full ERROR
traceback** and returns a generic **500** — instead of the clean **401** these
endpoints are documented to return for unauthenticated/unauthorized callers.
**Sentry:**
[SUPERSET-PYTHON-NMA](https://preset-inc.sentry.io/issues/5588396277/) —
`UserLookupError: No access`, culprit `CurrentUserRestApi.get_me`. **10,471
events / 31 users** (firstSeen 2024-07-09, still firing).
**Root cause (two overlapping gaps):**
1. **Regression from #36410** (merged 2025-12-11): before that PR, all three
methods carried manual `try/except NoAuthorizationError: return
self.response_401()` guards around `g.user` access. #36410 removed them, on the
assumption that `@protect()` fully covers auth failures. It does not —
flask_appbuilder's `protect()` performs no exception handling of its own around
the lazy `g.user`/`current_user` resolution that happens later, inside the body.
2. **Always-missing `UserLookupError` coverage:** even the old guards only
caught `NoAuthorizationError`. `UserLookupError` (the "member with no role"
path, which Preset treats as the *expected, routine* outcome for those users —
not a sync gap) was never handled here at all.
### FIX
Add explicit handling around the `g.user` access in all three
`CurrentUserRestApi` methods, catching both `NoAuthorizationError` and
`UserLookupError` (both from `flask_jwt_extended.exceptions`) and returning
`self.response_401()`. This mirrors the established precedent already in this
codebase — `superset/views/base.py`'s `api` decorator — which logs auth-outcome
exceptions at **WARNING** (not ERROR/`exc_info`-heavy stack spam) before
returning 401, reserving ERROR+500 for genuinely unexpected failures.
The change is deliberately surgical:
- `@protect()`/flask_appbuilder is third-party and untouched.
- Other endpoints in the file (e.g. `UserRestApi`/avatar) that were never
part of this bug are untouched.
- `update_me` already had a `try/except ValidationError` block, so the new
`except` clause was added alongside it rather than nesting a second `try`.
### TRADEOFFS
**None.** This strictly repairs a broken-in-a-worse-way-than-intended auth
response: a request that should always have been a `401` was instead emitting a
`500` plus a noisy ERROR traceback on every occurrence. No
legitimate-user-facing behavior changes — successful requests still return
`200` with identical payloads, and genuinely unexpected exceptions still fall
through to `@safe`'s ERROR+500 handler unchanged. The only observable
differences are (a) the correct `401` status for the affected auth outcomes and
(b) WARNING-level (instead of ERROR-level) logging for them.
### FOLLOW-UPS
None expected.
### TESTING INSTRUCTIONS
New unit tests: `tests/unit_tests/views/test_current_user_auth_errors.py`.
They drive the real endpoints via the test client (with `@protect()`
satisfied through the `full_api_access` fixture) and patch the module-level `g`
so that resolving `g.user` inside each body raises the target exception —
exactly as flask-login does in production — then assert the response is `401`.
Coverage: `get_me`, `get_my_roles`, and `update_me`, each against both
`UserLookupError` and `NoAuthorizationError` (6 cases).
**Non-vacuous verification (stash-the-fix / keep-the-tests):**
```
# WITH the fix:
$ pytest tests/unit_tests/views/test_current_user_auth_errors.py -q
......
[100%]
6 passed in 0.96s
# git stash push -- superset/views/users/api.py (revert only the fix, keep
the tests)
$ pytest tests/unit_tests/views/test_current_user_auth_errors.py -q
...
return self.response(200, result=user_response_schema.dump(g.user))
^^^^^^
flask_jwt_extended.exceptions.NoAuthorizationError: Missing Authorization
Header
6 failed in 1.18s
```
The pre-existing `tests/unit_tests/views/test_current_user_api.py` (4 tests)
continues to pass. `pre-commit run --files ...` (ruff, ruff-format, mypy,
pylint) passes on the changed files.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
---
Shortcut: [SC-120417](https://app.shortcut.com/preset/story/120417)
Fixes SUPERSET-PYTHON-NMA
--
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]
