(superset) 01/01: fix(api): return 401 not 500 for auth errors in CurrentUserRestApi (SC-120417)

[el...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

eschutho pushed a commit to branch fix-currentuser-userlookuperror-500
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 5062bb7949f501df2b79d4a1a726b4ed7b024635
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Sun Sep 13 15:13:41 2026 +0000

    fix(api): return 401 not 500 for auth errors in CurrentUserRestApi (SC-120417)
    
    The three /api/v1/me/ endpoints (get_me, get_my_roles, update_me) resolve
    g.user (a flask-login lazy proxy) inside their bodies. For a validly
    authenticated JWT whose bearer has no assigned role, that resolution raises
    UserLookupError; a missing/invalid authorization context raises
    NoAuthorizationError. Neither was caught: the exception propagated to
    flask_appbuilder's @safe decorator, which logs a full ERROR traceback and
    returns a generic 500 instead of a clean 401.
    
    PR #36410 removed the manual try/except guards these methods used to carry,
    assuming @protect() covers all auth failures -- it does not, and it never
    covered UserLookupError at all. Mirror the established views/base.py::api
    pattern: catch the auth-outcome exceptions, log at WARNING, and return 401.
    
    Fixes SUPERSET-PYTHON-NMA
---
 superset/views/users/api.py                        |  21 +++-
 .../views/test_current_user_auth_errors.py         | 122 +++++++++++++++++++++
 2 files changed, 140 insertions(+), 3 deletions(-)

diff --git a/superset/views/users/api.py b/superset/views/users/api.py
index ce2d53892be..cf3ed95a015 100644
--- a/superset/views/users/api.py
+++ b/superset/views/users/api.py
@@ -14,6 +14,7 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
+import logging
 from datetime import datetime
 from typing import Any, Dict
 
@@ -21,6 +22,7 @@ from flask import current_app as app, g, redirect, request, Response
 from flask_appbuilder.api import expose, permission_name, safe
 from flask_appbuilder.security.decorators import protect
 from flask_appbuilder.security.sqla.models import User
+from flask_jwt_extended.exceptions import NoAuthorizationError, UserLookupError
 from marshmallow import ValidationError
 from sqlalchemy.orm.exc import NoResultFound
 from werkzeug.security import check_password_hash, generate_password_hash
@@ -34,6 +36,8 @@ from superset.views.base_api import BaseSupersetApi, requires_json, statsd_metri
 from superset.views.users.schemas import CurrentUserPutSchema, UserResponseSchema
 from superset.views.utils import bootstrap_user_data
 
+logger = logging.getLogger(__name__)
+
 user_response_schema = UserResponseSchema()
 
 
@@ -115,7 +119,11 @@ class CurrentUserRestApi(BaseSupersetApi):
             401:
               $ref: '#/components/responses/401'
         """
-        return self.response(200, result=user_response_schema.dump(g.user))
+        try:
+            return self.response(200, result=user_response_schema.dump(g.user))
+        except (NoAuthorizationError, UserLookupError):
+            logger.warning("Api failed- no authorization", exc_info=True)
+            return self.response_401()
 
     @expose("/roles/", methods=("GET",))
     @protect()
@@ -142,8 +150,12 @@ class CurrentUserRestApi(BaseSupersetApi):
             401:
               $ref: '#/components/responses/401'
         """
-        user = bootstrap_user_data(g.user, include_perms=True)
-        return self.response(200, result=user)
+        try:
+            user = bootstrap_user_data(g.user, include_perms=True)
+            return self.response(200, result=user)
+        except (NoAuthorizationError, UserLookupError):
+            logger.warning("Api failed- no authorization", exc_info=True)
+            return self.response_401()
 
     @expose("/", methods=["PUT"])
     @protect()
@@ -194,6 +206,9 @@ class CurrentUserRestApi(BaseSupersetApi):
             return self.response(200, result=user_response_schema.dump(g.user))
         except ValidationError as error:
             return self.response_400(message=error.messages)
+        except (NoAuthorizationError, UserLookupError):
+            logger.warning("Api failed- no authorization", exc_info=True)
+            return self.response_401()
 
 
 class UserRestApi(BaseSupersetApi):
diff --git a/tests/unit_tests/views/test_current_user_auth_errors.py b/tests/unit_tests/views/test_current_user_auth_errors.py
new file mode 100644
index 00000000000..2d258fd87ef
--- /dev/null
+++ b/tests/unit_tests/views/test_current_user_auth_errors.py
@@ -0,0 +1,122 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for auth-outcome error handling in ``CurrentUserRestApi``.
+
+The three ``/api/v1/me/`` endpoints resolve ``g.user`` (a flask-login lazy
+proxy) inside their bodies. For a validly-authenticated JWT whose bearer has
+no assigned role, that resolution raises ``UserLookupError``; a missing/invalid
+authorization context raises ``NoAuthorizationError``. Neither is an
+"unexpected" server failure -- both are auth outcomes and must surface as a
+clean ``401``, not fall through to ``@safe``'s generic ``500`` handler.
+"""
+
+from __future__ import annotations
+
+from typing import Any, Callable
+
+import pytest
+from flask_jwt_extended.exceptions import NoAuthorizationError, UserLookupError
+from pytest_mock import MockerFixture
+
+
+class _RaisingG:
+    """Stand-in for the request-global whose ``user`` attribute raises.
+
+    Mirrors flask-login's lazy proxy: nothing fails until ``g.user`` is
+    actually dereferenced inside the endpoint body.
+    """
+
+    def __init__(self, exc: Exception) -> None:
+        self._exc = exc
+
+    @property
+    def user(self) -> Any:
+        raise self._exc
+
+
+# ``UserLookupError`` requires ``(message, jwt_header, jwt_data)``; the values
+# of the latter two are irrelevant to the handler under test, which keys only
+# off the exception type.
+_USER_LOOKUP_ERROR = UserLookupError("No access", {}, {})
+_NO_AUTH_ERROR = NoAuthorizationError("Missing Authorization Header")
+
+
+@pytest.fixture
+def raise_on_g_user(
+    mocker: MockerFixture,
+) -> Callable[[Exception], None]:
+    """Patch the module-level ``g`` in the users API so ``g.user`` raises.
+
+    Returns a setter the test calls with the exception it wants raised, so a
+    single fixture covers both the ``UserLookupError`` and
+    ``NoAuthorizationError`` cases.
+    """
+
+    def _set(exc: Exception) -> None:
+        mocker.patch("superset.views.users.api.g", _RaisingG(exc))
+
+    return _set
+
+
+@pytest.mark.parametrize(
+    "exc",
+    [_USER_LOOKUP_ERROR, _NO_AUTH_ERROR],
+    ids=["user_lookup_error", "no_authorization_error"],
+)
+@pytest.mark.parametrize(
+    "path",
+    ["/api/v1/me/", "/api/v1/me/roles/"],
+    ids=["get_me", "get_my_roles"],
+)
+def test_get_endpoints_return_401_not_500_on_auth_error(
+    client: Any,
+    full_api_access: None,
+    raise_on_g_user: Any,
+    path: str,
+    exc: Exception,
+) -> None:
+    """``GET /api/v1/me/`` and ``/api/v1/me/roles/`` translate an auth-outcome
+    exception raised while resolving ``g.user`` into a ``401`` -- not the
+    generic ``500`` that ``@safe`` would produce for an uncaught exception.
+    """
+    raise_on_g_user(exc)
+
+    response = client.get(path)
+
+    assert response.status_code == 401
+
+
+@pytest.mark.parametrize(
+    "exc",
+    [_USER_LOOKUP_ERROR, _NO_AUTH_ERROR],
+    ids=["user_lookup_error", "no_authorization_error"],
+)
+def test_update_me_returns_401_not_500_on_auth_error(
+    client: Any,
+    full_api_access: None,
+    raise_on_g_user: Any,
+    exc: Exception,
+) -> None:
+    """``PUT /api/v1/me/`` translates an auth-outcome exception raised while
+    resolving ``g.user`` into a ``401``. The existing ``ValidationError``
+    branch is unaffected: the new except clause lives alongside it.
+    """
+    raise_on_g_user(exc)
+
+    response = client.put("/api/v1/me/", json={"first_name": "Foo"})
+
+    assert response.status_code == 401