rusackas commented on code in PR #44017:
URL: https://github.com/apache/superset/pull/44017#discussion_r4140628324
##########
superset/subjects/utils.py:
##########
@@ -206,7 +206,23 @@ 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. Nothing reads a user's subjects after changing
+ them within a single request, so the cached set cannot go stale in place.
"""
+ if not has_request_context():
+ return _query_user_subject_ids(user_id)
+ cache: dict[int, list[int]] = g.setdefault("_user_subject_ids", {})
Review Comment:
Good catch, this is already keyed off the request object now instead of `g`,
and there's a test pushing a fresh request inside the same app context to pin
it. Should hold up to the leak you reproduced.
##########
superset/subjects/utils.py:
##########
@@ -206,7 +206,33 @@ 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. Nothing reads a user's subjects after changing
+ them within a single request, so the cached set cannot go stale in place.
Review Comment:
Fair, and this is fixed too. The docstring now says the actual reason (a
subject created mid-request isn't in anyone's editors/viewers yet) instead of
the "nothing reads it back" claim.
##########
tests/unit_tests/subjects/test_utils.py:
##########
@@ -611,3 +612,63 @@ 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:
Review Comment:
`test_get_user_subject_ids_serves_a_stale_set_within_the_request` does
exactly this now, patch returns `[7]`, read it, switch to `[7, 99]`, same
request still sees `[7]`. Thanks for spelling it out.
##########
superset/subjects/utils.py:
##########
@@ -206,7 +206,37 @@ 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
Review Comment:
Agreed the guest path deserves its own change rather than folding it in
here. Added a line to the docstring noting `is_viewer`'s guest branch falls
through to `subjects_from_roles` and isn't covered, so it doesn't get mistaken
for already handled.
##########
superset/subjects/utils.py:
##########
@@ -206,7 +206,42 @@ 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: the only path that reads it back is
+ ``ensure_no_lockout`` in ``commands/utils.py``, where a stale membership
can
+ at most let the caller lock themselves out -- annoying, not a security
+ hole. Neither direction flips a decision for a third party, and the
+ staleness window is bounded by the request.
Review Comment:
Went ahead and applied it in 215ec8a, matches your wording almost verbatim.
Nice catch on the reader list too, that was stale.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
