fix(auth): redirect anonymous attempts to view dashboard with next (#35345)

This commit is contained in:
Sam Firke
2025-10-16 16:33:37 -04:00
committed by GitHub
parent 408f84aea6
commit aede3bb5ba
7 changed files with 155 additions and 25 deletions

View File

@@ -27,7 +27,7 @@ import {
Typography,
Icons,
} from '@superset-ui/core/components';
import { useState, useEffect } from 'react';
import { useState, useEffect, useMemo } from 'react';
import { capitalize } from 'lodash/fp';
import { addDangerToast } from 'src/components/MessageToasts/actions';
import { useDispatch } from 'react-redux';
@@ -82,6 +82,26 @@ export default function Login() {
const dispatch = useDispatch();
const bootstrapData = getBootstrapData();
const nextUrl = useMemo(() => {
try {
const params = new URLSearchParams(window.location.search);
return params.get('next') || '';
} catch (_error) {
return '';
}
}, []);
const loginEndpoint = useMemo(
() => (nextUrl ? `/login/?next=${encodeURIComponent(nextUrl)}` : '/login/'),
[nextUrl],
);
const buildProviderLoginUrl = (providerName: string) => {
const base = `/login/${providerName}`;
return nextUrl
? `${base}${base.includes('?') ? '&' : '?'}next=${encodeURIComponent(nextUrl)}`
: base;
};
const authType: AuthType = bootstrapData.common.conf.AUTH_TYPE;
const providers: Provider[] = bootstrapData.common.conf.AUTH_PROVIDERS;
@@ -109,7 +129,7 @@ export default function Login() {
sessionStorage.setItem('login_attempted', 'true');
// Use standard form submission for Flask-AppBuilder compatibility
SupersetClient.postForm('/login/', values, '');
SupersetClient.postForm(loginEndpoint, values, '');
};
const getAuthIconElement = (
@@ -146,7 +166,7 @@ export default function Login() {
{providers.map((provider: OIDProvider) => (
<Form.Item<LoginForm>>
<Button
href={`/login/${provider.name}`}
href={buildProviderLoginUrl(provider.name)}
block
iconPosition="start"
icon={getAuthIconElement(provider.name)}
@@ -164,7 +184,7 @@ export default function Login() {
{providers.map((provider: OAuthProvider) => (
<Form.Item<LoginForm>>
<Button
href={`/login/${provider.name}`}
href={buildProviderLoginUrl(provider.name)}
block
iconPosition="start"
icon={getAuthIconElement(provider.name)}

View File

@@ -46,7 +46,6 @@ from sqlalchemy.exc import SQLAlchemyError
from werkzeug.utils import safe_join
from superset import (
appbuilder,
db,
event_logger,
is_feature_enabled,
@@ -80,6 +79,7 @@ from superset.models.slice import Slice
from superset.models.sql_lab import Query
from superset.models.user_attributes import UserAttribute
from superset.superset_typing import FlaskResponse
from superset.tasks.utils import get_current_user
from superset.utils import core as utils, json
from superset.utils.cache import etag_cache
from superset.utils.core import (
@@ -108,6 +108,7 @@ from superset.views.utils import (
get_form_data,
get_viz,
loads_request_json,
redirect_to_login,
sanitize_datasource_data,
)
from superset.viz import BaseViz
@@ -765,13 +766,21 @@ class Superset(BaseSupersetView):
dashboard = Dashboard.get(dashboard_id_or_slug)
if not dashboard:
if not get_current_user():
return redirect_to_login()
abort(404)
# Redirect anonymous users to login for unpublished dashboards,
# in the edge case where a dataset has been shared with public
if not get_current_user() and not dashboard.published:
return redirect_to_login()
try:
dashboard.raise_for_access()
except SupersetSecurityException:
# Return 404 to avoid revealing dashboard existence
return Response(status=404)
if not get_current_user():
return redirect_to_login()
abort(404)
add_extra_log_payload(
dashboard_id=dashboard.id,
dashboard_version="v2",
@@ -882,7 +891,7 @@ class Superset(BaseSupersetView):
def welcome(self) -> FlaskResponse:
"""Personalized welcome page"""
if not g.user or not get_user_id():
return redirect(appbuilder.get_url_for_login)
return redirect_to_login()
if welcome_dashboard_id := (
db.session.query(UserAttribute.welcome_dashboard_id)

View File

@@ -25,7 +25,6 @@ from typing import Any, Callable, cast
from flask import (
Flask,
redirect,
request,
Response,
send_file,
@@ -34,7 +33,6 @@ from flask_wtf.csrf import CSRFError
from sqlalchemy import exc
from werkzeug.exceptions import HTTPException
from superset import appbuilder
from superset.commands.exceptions import CommandException, CommandInvalidError
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import (
@@ -46,6 +44,7 @@ from superset.exceptions import (
from superset.superset_typing import FlaskResponse
from superset.utils import core as utils, json
from superset.utils.log import get_logger_from_status
from superset.views.utils import redirect_to_login
if typing.TYPE_CHECKING:
from superset.views.base import BaseSupersetView
@@ -153,7 +152,7 @@ def set_app_error_handlers(app: Flask) -> None: # noqa: C901
if request.is_json:
return show_http_exception(ex)
return redirect(appbuilder.get_url_for_login)
return redirect_to_login()
@app.errorhandler(HTTPException)
def show_http_exception(ex: HTTPException) -> FlaskResponse:
@@ -165,7 +164,11 @@ def set_app_error_handlers(app: Flask) -> None: # noqa: C901
and ex.code in {404, 500}
):
path = files("superset") / f"static/assets/{ex.code}.html"
return send_file(path, max_age=0), ex.code
# Try to serve HTML file; fall back to JSON if not built
try:
return send_file(path, max_age=0), ex.code
except FileNotFoundError:
pass
return json_error_response(
[
@@ -189,7 +192,11 @@ def set_app_error_handlers(app: Flask) -> None: # noqa: C901
if "text/html" in request.accept_mimetypes and not app.config["DEBUG"]:
path = files("superset") / "static/assets/500.html"
return send_file(path, max_age=0), 500
# Try to serve HTML file; fall back to JSON if not built
try:
return send_file(path, max_age=0), 500
except FileNotFoundError:
pass
extra = ex.normalized_messages() if isinstance(ex, CommandInvalidError) else {}
return json_error_response(

View File

@@ -19,16 +19,17 @@ import logging
from collections import defaultdict
from functools import wraps
from typing import Any, Callable, DefaultDict, Optional, Union
from urllib import parse
import msgpack
import pyarrow as pa
from flask import current_app as app, g, has_request_context, request
from flask import current_app as app, g, has_request_context, redirect, request
from flask_appbuilder.security.sqla import models as ab_models
from flask_appbuilder.security.sqla.models import User
from flask_babel import _
from sqlalchemy.exc import NoResultFound
from superset import dataframe, db, result_set, viz
from superset import appbuilder, dataframe, db, result_set, viz
from superset.common.db_query_status import QueryStatus
from superset.daos.datasource import DatasourceDAO
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
@@ -44,7 +45,7 @@ from superset.models.core import Database
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice
from superset.models.sql_lab import Query
from superset.superset_typing import FormData
from superset.superset_typing import FlaskResponse, FormData
from superset.utils import json
from superset.utils.core import DatasourceType
from superset.utils.decorators import stats_timing
@@ -58,6 +59,33 @@ if not feature_flag_manager.is_feature_enabled("ENABLE_JAVASCRIPT_CONTROLS"):
REJECTED_FORM_DATA_KEYS = ["js_tooltip", "js_onclick_href", "js_data_mutator"]
def redirect_to_login(next_target: str | None = None) -> FlaskResponse:
"""Return a redirect response to the login view, preserving target URL.
When ``next_target`` is ``None`` the current request path (including query
string) is used, provided a request context is available. The resulting URL
always remains relative, mirroring Flask-AppBuilder expectations.
"""
login_url = appbuilder.get_url_for_login
parsed = parse.urlparse(login_url)
query = parse.parse_qs(parsed.query, keep_blank_values=True)
target = next_target
if target is None and has_request_context():
if request.query_string:
target = request.full_path.rstrip("?")
else:
target = request.path
if target:
query["next"] = [target]
encoded_query = parse.urlencode(query, doseq=True)
redirect_url = parse.urlunparse(parsed._replace(query=encoded_query))
return redirect(redirect_url)
def sanitize_datasource_data(datasource_data: dict[str, Any]) -> dict[str, Any]:
if datasource_data:
datasource_database = datasource_data.get("database")

View File

@@ -18,8 +18,8 @@
"""Unit tests for Superset"""
import re
import unittest
from random import random
from urllib.parse import parse_qs, urlparse
import pytest
from flask import Response, escape, url_for
@@ -52,7 +52,7 @@ from tests.integration_tests.fixtures.world_bank_dashboard import (
load_world_bank_data, # noqa: F401
)
from .base_tests import SupersetTestCase
from .base_tests import DEFAULT_PASSWORD, SupersetTestCase
class TestDashboard(SupersetTestCase):
@@ -186,6 +186,72 @@ class TestDashboard(SupersetTestCase):
# Cleanup
self.revoke_public_access_to_table(table)
@pytest.mark.usefixtures(
"load_energy_table_with_slice",
"load_dashboard",
)
def test_anonymous_user_redirects_to_login_with_next(self):
self.logout()
target_path = f"/superset/dashboard/{pytest.hidden_dash_slug}/"
response = self.client.get(target_path, follow_redirects=False)
assert response.status_code == 302
redirect_location = response.headers["Location"]
parsed = urlparse(redirect_location)
assert parsed.path.rstrip("/") == "/login"
next_values = parse_qs(parsed.query).get("next")
assert next_values is not None
assert next_values[0].endswith(target_path)
login_target = (
f"{parsed.path}{'?' + parsed.query if parsed.query else ''}"
if parsed.scheme or parsed.netloc
else redirect_location
)
login_response = self.client.post(
login_target,
data={"username": ADMIN_USERNAME, "password": DEFAULT_PASSWORD},
follow_redirects=False,
)
assert login_response.status_code == 302
assert login_response.headers["Location"].endswith(target_path)
target_response: Response = self.client.get(target_path, follow_redirects=False)
assert target_response.status_code == 200
def test_anonymous_user_redirects_to_login_for_missing_dashboard(self):
self.logout()
target_path = "/superset/dashboard/nonexistent-dashboard/"
response = self.client.get(target_path, follow_redirects=False)
assert response.status_code == 302
parsed = urlparse(response.headers["Location"])
assert parsed.path.rstrip("/") == "/login"
next_values = parse_qs(parsed.query).get("next")
assert next_values is not None
assert next_values[0].endswith(target_path)
@pytest.mark.usefixtures(
"public_role_like_gamma",
"load_energy_table_with_slice",
"load_dashboard",
)
def test_authenticated_user_without_access_gets_404(self):
self.login(GAMMA_USERNAME)
target_path = f"/superset/dashboard/{pytest.hidden_dash_slug}/"
response = self.client.get(
target_path,
follow_redirects=False,
headers={"Accept": "text/html"},
)
assert response.status_code == 404
@pytest.mark.usefixtures(
"public_role_like_gamma",
"load_energy_table_with_slice",
@@ -248,7 +314,3 @@ class TestDashboard(SupersetTestCase):
db.session.commit()
assert f"/superset/dashboard/{slug}/" not in resp
if __name__ == "__main__":
unittest.main()

View File

@@ -108,7 +108,7 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
# act
response = self.get_dashboard_view_response(dashboard_to_access)
assert response.status_code == 404
assert response.status_code == 404 # Authenticated users without access get 404
request_payload = get_query_context("birth_names")
rv = self.post_assert_metric(CHART_DATA_URI, request_payload, "data")
@@ -221,7 +221,8 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
response = self.get_dashboard_view_response(dashboard_to_access)
# assert
assert response.status_code == 404
# Anonymous users are redirected to login instead of getting 404
assert response.status_code == 302
@pytest.mark.usefixtures("public_role_like_gamma")
def test_get_dashboard_view__public_user_with_dashboard_permission_can_not_access_draft( # noqa: E501
@@ -234,7 +235,8 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
response = self.get_dashboard_view_response(dashboard_to_access)
# assert
assert response.status_code == 404
# Anonymous users are redirected to login for unpublished dashboards
assert response.status_code == 302
# post
revoke_access_to_dashboard(dashboard_to_access, "Public") # noqa: F405

View File

@@ -190,6 +190,8 @@ def delete_all_inserted_objects() -> None:
def delete_all_inserted_dashboards():
try:
# Expire all objects to ensure fresh state after potential rollbacks
db.session.expire_all()
dashboards_to_delete: list[Dashboard] = (
db.session.query(Dashboard)
.filter(Dashboard.id.in_(inserted_dashboards_ids))