(superset) 01/01: fix(security_manager): stop SupersetAuthView from shadowing AUTH_REMOTE_USER

[ru...@apache.org]

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

rusackas pushed a commit to branch fix/auth-remote-user-superset-auth-view-shadow-36117
in repository https://gitbox.apache.org/repos/asf/superset.git

commit cdbe37587f29deae3ea6bbb128a88f192daffb95
Author: rusackas <ev...@rusackas.com>
AuthorDate: Sun Aug 9 18:00:31 2026 -0700

    fix(security_manager): stop SupersetAuthView from shadowing AUTH_REMOTE_USER
    
    SupersetSecurityManager.register_views() unconditionally registers
    SupersetAuthView at "/login/" before calling super().register_views(),
    which is where FlaskAppBuilder picks the auth_type-specific view (e.g.
    AuthRemoteUserView for AUTH_REMOTE_USER) and registers it at the same
    route. Both views add distinct Flask endpoints for the same URL rule, and
    SupersetAuthView always wins the dispatch since it's registered first, so
    the SPA login shell renders instead of AuthRemoteUserView ever checking
    the remote-user header/env var. AUTH_REMOTE_USER is silently a no-op as a
    result: plain AUTH_TYPE = AUTH_REMOTE_USER config (unchanged since it
    worked in 5.0.0) stops logging anyone in once SupersetAuthView exists.
    
    This is a regression from the frontend login migration (#33255, part of
    the 6.0 theming overhaul). #39098 added an opt-out flag
    (register_superset_auth_view = False) for custom SecurityManager
    subclasses, but that requires writing and wiring up a subclass just to
    get the out-of-the-box config working again -- not something a deployer
    setting plain AUTH_TYPE = AUTH_REMOTE_USER would know to do, and not
    documented anywhere. Confirmed still broken on 6.1.0 in the issue thread.
    
    Skip registering SupersetAuthView specifically for AUTH_REMOTE_USER, whose
    whole point is a non-interactive, header-driven login -- there's no
    legitimate use for the SPA shell there. Every other auth type (DB, OAuth,
    LDAP, SAML) is unaffected.
    
    Fixes #36117
---
 superset/security/manager.py              | 15 +++++-
 tests/unit_tests/security/manager_test.py | 76 +++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/superset/security/manager.py b/superset/security/manager.py
index 5dc044d4a89..410eeace8cd 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -38,6 +38,7 @@ from flask import current_app, Flask, g, has_app_context, Request, Response
 from flask_appbuilder import Model
 from flask_appbuilder.api import expose, protect, safe
 from flask_appbuilder.models.filters import BaseFilter
+from flask_appbuilder.security.manager import AUTH_REMOTE_USER
 from flask_appbuilder.security.sqla.apis import GroupApi, RoleApi, UserApi
 from flask_appbuilder.security.sqla.apis.permission_view_menu.api import (
     PermissionViewMenuApi,
@@ -5048,7 +5049,19 @@ class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
     def register_views(self) -> None:
         from superset.views.auth import SupersetAuthView, SupersetRegisterUserView
 
-        if self.register_superset_auth_view:
+        # AUTH_REMOTE_USER has no interactive login form to render: the whole
+        # point is that an upstream proxy already authenticated the request and
+        # passes the identity via a header/env var, so FlaskAppBuilder's
+        # AuthRemoteUserView performs a silent GET-time login with no UI. That
+        # view registers at the same "/login/" route as SupersetAuthView, and
+        # since both add distinct Flask endpoints for the same URL rule,
+        # whichever gets registered first wins the dispatch -- SupersetAuthView
+        # always wins because it's added before super().register_views() runs
+        # FlaskAppBuilder's own auth_type dispatch. That silently shadows
+        # AUTH_REMOTE_USER: the SPA login shell renders instead of the header
+        # ever being checked. Skip registering it for this auth type so
+        # FlaskAppBuilder's AuthRemoteUserView actually claims the route.
+        if self.register_superset_auth_view and self.auth_type != AUTH_REMOTE_USER:
             self.auth_view = self.appbuilder.add_view_no_menu(SupersetAuthView)
         if self.register_superset_registeruser_view:
             self.registeruser_view = self.appbuilder.add_view_no_menu(
diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py
index b940225c6cb..2f06397725a 100644
--- a/tests/unit_tests/security/manager_test.py
+++ b/tests/unit_tests/security/manager_test.py
@@ -24,6 +24,7 @@ from typing import Any, Optional
 from unittest.mock import MagicMock
 
 import pytest
+from flask_appbuilder.const import AUTH_DB, AUTH_REMOTE_USER
 from flask_appbuilder.security.sqla.models import Role, User
 from pytest_mock import MockerFixture
 
@@ -51,6 +52,81 @@ def test_security_manager(app_context: None) -> None:
     assert sm
 
 
+def _register_views_with_mock_appbuilder(
+    mocker: MockerFixture, auth_type: int
+) -> MagicMock:
+    """
+    Build a SupersetSecurityManager bound to a fresh mock appbuilder and call
+    register_views() on it, with FlaskAppBuilder's own register_views (the
+    super() call, which does its own large auth_type dispatch and permission
+    registration) stubbed out so the test stays scoped to just the override.
+    Returns the mock appbuilder so the caller can inspect what got registered.
+    """
+    from flask import current_app
+
+    current_app.config["AUTH_TYPE"] = auth_type
+    current_app.config["AUTH_USER_REGISTRATION"] = False
+    current_app.config["AUTH_RATE_LIMITED"] = False
+
+    mock_appbuilder = mocker.MagicMock()
+    mock_appbuilder.baseviews = []
+    mock_appbuilder.menu.get_list.return_value = []
+
+    sm = SupersetSecurityManager.__new__(SupersetSecurityManager)
+    sm.appbuilder = mock_appbuilder
+    sm.register_superset_auth_view = True
+    sm.register_superset_registeruser_view = False
+    sm.userstatschartview = None
+
+    mocker.patch(
+        "flask_appbuilder.security.sqla.manager.SecurityManager.register_views",
+        autospec=True,
+    )
+
+    sm.register_views()
+    return mock_appbuilder
+
+
+def test_register_views_does_not_shadow_auth_remote_user(
+    app_context: None, mocker: MockerFixture
+) -> None:
+    """
+    AUTH_REMOTE_USER must not register SupersetAuthView at "/login/".
+
+    AuthRemoteUserView performs a silent, header-driven login with no
+    interactive UI. SupersetAuthView (the SPA login shell) previously
+    registered at the same route unconditionally and always won the routing
+    dispatch, so the remote-user header was never even checked -- reported in
+    apache/superset#36117 as a regression from the frontend login migration
+    (#31590). See also the related opt-out added in #39098.
+    """
+    from superset.views.auth import SupersetAuthView
+
+    mock_appbuilder = _register_views_with_mock_appbuilder(mocker, AUTH_REMOTE_USER)
+
+    registered = [
+        call.args[0] for call in mock_appbuilder.add_view_no_menu.call_args_list
+    ]
+    assert SupersetAuthView not in registered
+
+
+def test_register_views_still_registers_superset_auth_view_for_db_auth(
+    app_context: None, mocker: MockerFixture
+) -> None:
+    """
+    Control case: AUTH_DB (the common, interactive case) is unaffected --
+    SupersetAuthView still registers at "/login/" as before.
+    """
+    from superset.views.auth import SupersetAuthView
+
+    mock_appbuilder = _register_views_with_mock_appbuilder(mocker, AUTH_DB)
+
+    registered = [
+        call.args[0] for call in mock_appbuilder.add_view_no_menu.call_args_list
+    ]
+    assert SupersetAuthView in registered
+
+
 @pytest.fixture
 def stored_metrics() -> list[AdhocMetric]:
     """