You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "eschutho (via GitHub)" <gi...@apache.org> on 2026/09/13 15:14:29 UTC
[PR] fix(api): return 401 not 500 for auth errors in CurrentUserRestApi (SC-120417) [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(api): return 401 not 500 for auth errors in CurrentUserRestApi (SC-120417) [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44213:
URL: https://github.com/apache/superset/pull/44213#issuecomment-5654155340
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44213?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `17.64706%` with `14 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.44%. Comparing base ([`8e66145`](https://app.codecov.io/gh/apache/superset/commit/8e66145cfed5ca0584cc71c7795814a2752522e6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5062bb7`](https://app.codecov.io/gh/apache/superset/commit/5062bb7949f501df2b79d4a1a726b4ed7b024635?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44213?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/views/users/api.py](https://app.codecov.io/gh/apache/superset/pull/44213?src=pr&el=tree&filepath=superset%2Fviews%2Fusers%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvdXNlcnMvYXBpLnB5) | 17.64% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44213?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (8e66145) and HEAD (5062bb7). Click for more details.
>
> <details><summary>HEAD has 115 uploads less than BASE</summary>
>
>| Flag | BASE (8e66145) | HEAD (5062bb7) |
>|------|------|------|
>|hive|15|1|
>|python|89|2|
>|presto|15|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44213 +/- ##
===========================================
- Coverage 80.17% 57.44% -22.74%
===========================================
Files 2925 2925
Lines 172670 172684 +14
Branches 40087 40087
===========================================
- Hits 138440 99190 -39250
- Misses 31633 72554 +40921
+ Partials 2597 940 -1657
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44213/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44213/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.33% <17.64%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44213/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44213/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44213/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.21% <17.64%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44213/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.26% <17.64%> (-45.30%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44213/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44213/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44213?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(api): return 401 not 500 for auth errors in CurrentUserRestApi (SC-120417) [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #44213:
URL: https://github.com/apache/superset/pull/44213
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(api): return 401 not 500 for auth errors in CurrentUserRestApi (SC-120417) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44213:
URL: https://github.com/apache/superset/pull/44213#issuecomment-5654127172
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(api): return 401 not 500 for auth errors in CurrentUserRestApi (SC-120417) [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #44213:
URL: https://github.com/apache/superset/pull/44213#issuecomment-5654195982
@richardfogaca assigning you as reviewer here. This is the auth-error handling on the `/api/v1/me/` endpoints in `superset/views/users/api.py` — part of the Sentry raw-500 → clean-4xx cleanup series (SC-120417). Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org