This is an automated email from the ASF dual-hosted git repository.
EnxDev pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 3f8a23622c7 perf(security): memoise the user subject lookup within a
request (#44017)
3f8a23622c7 is described below
commit 3f8a23622c76e5423fbec7746102e6b03614da42
Author: shaurya <[email protected]>
AuthorDate: Wed Sep 30 19:48:14 2026 +0530
perf(security): memoise the user subject lookup within a request (#44017)
Co-authored-by: Shaurya <[email protected]>
Co-authored-by: Evan Rusackas <[email protected]>
Co-authored-by: Evan Rusackas <[email protected]>
Co-authored-by: Claude Sonnet 5 <[email protected]>
---
superset/subjects/utils.py | 40 ++++++++++++++++-
tests/unit_tests/subjects/test_utils.py | 80 +++++++++++++++++++++++++++++++++
2 files changed, 119 insertions(+), 1 deletion(-)
diff --git a/superset/subjects/utils.py b/superset/subjects/utils.py
index b6cccc8150c..c09fa04449c 100644
--- a/superset/subjects/utils.py
+++ b/superset/subjects/utils.py
@@ -22,7 +22,7 @@ if TYPE_CHECKING:
from flask_appbuilder.security.sqla.models import Group, Role, User
from sqlalchemy.sql import CompoundSelect, Select
-from flask import has_app_context
+from flask import has_app_context, has_request_context, request
from sqlalchemy import select, union_all
from superset import db
@@ -206,7 +206,45 @@ def get_user_subject_ids(user_id: int) -> list[int]:
1. The user's own USER-type subject
2. ROLE-type subjects for all direct and group-derived roles the user has
3. GROUP-type subjects for all groups the user belongs to
+
+ Memoised for the duration of the request, keyed by user id. Authorization
+ calls this once per object checked -- ``is_editor``/``is_viewer`` run it
for
+ every chart on a dashboard. The cache can miss a subject created earlier in
+ the same request (the create paths in ``commands/utils.py`` read back
through
+ here), but that never grants access to anyone else, in either direction. A
+ cache that misses a just-created subject: the subject is not yet listed in
+ any resource's editors or viewers, so an ``is_editor``/``is_viewer`` check
+ still returns the same answer. A cache that retains a subject removed
+ earlier in the same request: membership only changes in the Admin user,
+ role and group endpoints or in login-time role sync, and none of those
+ requests read this back afterwards. Neither direction flips a decision
+ for a third party, and the staleness window is bounded by the request.
+
+ The cache lives on ``flask.request``, not ``g``: ``g`` is bound to the app
+ context, which a worker can hold open across many requests, so a
``g``-keyed
+ cache would survive past the request it was built for. The request object
is
+ torn down when the request ends, giving the cache exactly the request
+ lifetime the docstring promises.
+
+ Guest users don't reach this cache: ``is_viewer`` falls through to
+ ``subjects_from_roles`` for them, still paying a query per role per
+ resource. That path is out of scope here.
"""
+ if not has_request_context():
+ return _query_user_subject_ids(user_id)
+ req = request._get_current_object()
+ cache: dict[int, list[int]] | None = getattr(req, "_user_subject_ids",
None)
+ if cache is None:
+ cache = {}
+ req._user_subject_ids = cache
+ if user_id not in cache:
+ cache[user_id] = _query_user_subject_ids(user_id)
+ # Copy: callers hand this list on, and one of them puts it in the bootstrap
+ # payload that COMMON_BOOTSTRAP_OVERRIDES_FUNC can edit.
+ return list(cache[user_id])
+
+
+def _query_user_subject_ids(user_id: int) -> list[int]:
result = db.session.execute(get_user_subject_ids_subquery(user_id))
return [row[0] for row in result]
diff --git a/tests/unit_tests/subjects/test_utils.py
b/tests/unit_tests/subjects/test_utils.py
index e5ebf4323e5..722a26d5ead 100644
--- a/tests/unit_tests/subjects/test_utils.py
+++ b/tests/unit_tests/subjects/test_utils.py
@@ -34,6 +34,7 @@ from superset.subjects.utils import (
get_or_create_group_subject,
get_or_create_role_subject,
get_user_group_subjects,
+ get_user_subject_ids,
get_user_subject_ids_subquery,
)
@@ -611,3 +612,82 @@ def test_compute_subjects_all_variants(mock_compute):
ensure_no_lockout=True,
field_name="editors",
)
+
+
+def test_get_user_subject_ids_memoises_within_a_request(app) -> None:
+ """The subject lookup runs once per user per request, not once per
check."""
+ with patch(
+ "superset.subjects.utils._query_user_subject_ids", return_value=[7, 8]
+ ) as query:
+ with app.test_request_context("/"):
+ assert get_user_subject_ids(1) == [7, 8]
+ assert get_user_subject_ids(1) == [7, 8]
+ assert query.call_count == 1
+
+ # A different principal is a different cache entry.
+ get_user_subject_ids(2)
+ assert query.call_count == 2
+
+
+def test_get_user_subject_ids_cache_does_not_leak_across_requests(app) -> None:
+ """The cache is tied to the request, not the enclosing app context.
+
+ A worker can hold one app context open across many requests. If the cache
+ lived on ``g`` it would survive from one request into the next and serve a
+ stale answer; tied to the request object it starts empty each time.
+ """
+ with app.app_context():
+ with patch(
+ "superset.subjects.utils._query_user_subject_ids",
return_value=[7, 8]
+ ) as query:
+ with app.test_request_context("/"):
+ assert get_user_subject_ids(1) == [7, 8]
+ assert get_user_subject_ids(1) == [7, 8]
+ assert query.call_count == 1
+
+ # Same app context, a new request. The membership changed underneath;
+ # the second request must re-query rather than reuse the first's cache.
+ with patch(
+ "superset.subjects.utils._query_user_subject_ids", return_value=[9]
+ ) as query:
+ with app.test_request_context("/"):
+ assert get_user_subject_ids(1) == [9]
+ assert query.call_count == 1
+
+
+def test_get_user_subject_ids_not_cached_outside_a_request(app_context) ->
None:
+ """Background tasks and CLI commands keep the uncached behaviour."""
+ with patch(
+ "superset.subjects.utils._query_user_subject_ids", return_value=[7]
+ ) as query:
+ get_user_subject_ids(1)
+ get_user_subject_ids(1)
+ assert query.call_count == 2
+
+
+def test_get_user_subject_ids_returns_a_copy(app) -> None:
+ """Callers hand this list on, so mutating it must not poison the cache."""
+ with patch("superset.subjects.utils._query_user_subject_ids",
return_value=[7]):
+ with app.test_request_context("/"):
+ first = get_user_subject_ids(1)
+ first.append(999)
+ assert get_user_subject_ids(1) == [7]
+
+
+def test_get_user_subject_ids_serves_a_stale_set_within_the_request(app) ->
None:
+ """A subject added mid-request is not seen until the next request.
+
+ This is the staleness window the docstring relies on: a create path can add
+ a subject after the cache is warm, and this call keeps returning the set it
+ first saw. It is safe because a subject created mid-request is not yet
listed
+ in any resource's editors or viewers, so a check that omits it returns the
+ same access decision.
+ """
+ with app.test_request_context("/"):
+ with patch("superset.subjects.utils._query_user_subject_ids",
return_value=[7]):
+ assert get_user_subject_ids(1) == [7]
+ # Membership changed underneath, same request: the cache still answers
7.
+ with patch(
+ "superset.subjects.utils._query_user_subject_ids",
return_value=[7, 99]
+ ):
+ assert get_user_subject_ids(1) == [7]
