codeant-ai-for-open-source[bot] commented on code in PR #42122:
URL: https://github.com/apache/superset/pull/42122#discussion_r3600405177
##########
tests/unit_tests/jinja_context_test.py:
##########
@@ -684,6 +684,104 @@ def test_user_macros_without_user_info(mocker:
MockerFixture):
assert cache.current_user_rls_rules() is None
+def _user_metadata_cache_keys(
+ mocker: MockerFixture,
+ *,
+ user_id: int | None,
+ username: str | None,
+ email: str | None,
+ roles: list[str],
+) -> list[Any]:
+ """
+ Render the user-metadata macros for a given user and return the values they
+ contributed to the query cache key.
+ """
+ mock_g = mocker.patch("superset.utils.core.g")
+ if user_id is None:
+ mock_g.user = None
+ else:
+ mock_g.user.id = user_id
+ mock_g.user.username = username
+ mock_g.user.email = email
+ mocker.patch(
+ "superset.security_manager.get_user_roles",
+ return_value=[Role(name=name) for name in roles],
+ )
+ keys: list[Any] = []
+ cache = ExtraCache(extra_cache_keys=keys, table=mocker.MagicMock())
+ cache.current_user_id()
+ cache.current_username()
+ cache.current_user_email()
+ cache.current_user_roles()
+ return keys
+
+
+def test_user_metadata_cache_keys_isolate_distinct_users(mocker:
MockerFixture):
+ """
+ Two different users contribute disjoint values to the cache key, so neither
+ can be served the other's cached result. This is the property that keeps
the
+ ``current_user_*`` macro family safe for per-user (and multi-tenant)
queries.
+ """
+ alice = _user_metadata_cache_keys(
+ mocker, user_id=1, username="alice", email="[email protected]",
roles=["Admin"]
+ )
+ bob = _user_metadata_cache_keys(
+ mocker, user_id=2, username="bob", email="[email protected]",
roles=["Gamma"]
+ )
+ assert alice
+ assert bob
+ assert set(alice).isdisjoint(set(bob))
+
+
+def test_user_metadata_cache_keys_match_for_identical_users(mocker:
MockerFixture):
+ """
+ The same user always contributes the same values, so identical renders
+ correctly share a cache entry (no needless fragmentation).
+ """
+ first = _user_metadata_cache_keys(
+ mocker, user_id=1, username="alice", email="[email protected]",
roles=["Admin"]
+ )
+ second = _user_metadata_cache_keys(
+ mocker, user_id=1, username="alice", email="[email protected]",
roles=["Admin"]
+ )
+ assert first == second
+
+
+def test_anonymous_user_never_collides_with_a_logged_in_user(mocker:
MockerFixture):
+ """
+ Refutes the "skip cache key when the value is absent" collision concern: an
+ anonymous render contributes nothing to the cache key, so its key can never
+ equal a logged-in user's, and no logged-in user's cached data is served to
an
+ anonymous request. Every absent user also renders identically (the macros
+ return ``None``), so absent users correctly share one cache entry rather
than
+ colliding.
+ """
+ logged_in = _user_metadata_cache_keys(
+ mocker, user_id=1, username="alice", email="[email protected]",
roles=["Admin"]
+ )
+ anonymous = _user_metadata_cache_keys(
+ mocker, user_id=None, username=None, email=None, roles=[]
+ )
+ assert anonymous == []
Review Comment:
**Suggestion:** This test hard-codes the anonymous-role path to `roles=[]`
and then asserts the anonymous cache-key contribution is empty, but in real
behavior anonymous users can contribute the public role via
`current_user_roles()`. That means the test can pass while not exercising the
actual anonymous contract, creating false confidence about collision behavior.
Model a real anonymous user (`is_anonymous=True`) and assert against the real
role contribution (or explicitly scope the test to the “no user object” case)
instead of forcing an empty roles list. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Anonymous cache-key test misrepresents real public-role behavior.
⚠️ Collision-safety for anonymous users lacks realistic verification.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset/jinja_context.py:88-103` and see
`ExtraCache.current_user_roles()` calls
`security_manager.get_user_roles()`, JSON-dumps the sorted role names, and
appends them to
`extra_cache_keys` when the list is non-empty.
2. Open `superset/security/manager.py:7-13` (around line 4154) and observe
`BaseSecurityManager.get_user_roles()` returns `[self.get_public_role()]` for
`user.is_anonymous` when `AUTH_ROLE_PUBLIC` is configured, meaning real
anonymous users
can contribute a public role to the cache key.
3. Open `tests/unit_tests/jinja_context_test.py:38-67` (diff lines 687-716)
and note
`_user_metadata_cache_keys()` patches
`superset.security_manager.get_user_roles` to return
`[Role(name=name) for name in roles]` and, when `user_id is None`, sets
`mock_g.user =
None`, bypassing the real anonymous-user path.
4. In the same file at
`test_anonymous_user_never_collides_with_a_logged_in_user` (diff
lines 750-767), see that the anonymous case is constructed with
`user_id=None` and
`roles=[]`, then `assert anonymous == []` checks that no cache-key values
were
contributed; this passes because the test stub forces `get_user_roles()` to
return an
empty list and `g.user` is `None`, even though in real anonymous execution
`current_user_roles()` may add the public role to the cache key, so the test
does not
exercise or validate the actual anonymous-role behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=face0fb20cc84d289063ec0cc3f343a5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=face0fb20cc84d289063ec0cc3f343a5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/jinja_context_test.py
**Line:** 762:765
**Comment:**
*Logic Error: This test hard-codes the anonymous-role path to
`roles=[]` and then asserts the anonymous cache-key contribution is empty, but
in real behavior anonymous users can contribute the public role via
`current_user_roles()`. That means the test can pass while not exercising the
actual anonymous contract, creating false confidence about collision behavior.
Model a real anonymous user (`is_anonymous=True`) and assert against the real
role contribution (or explicitly scope the test to the “no user object” case)
instead of forcing an empty roles list.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42122&comment_hash=8670cb4bfb1f864a84809f512c2ec9074e6e1245c4c69047888d824d374042f0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42122&comment_hash=8670cb4bfb1f864a84809f512c2ec9074e6e1245c4c69047888d824d374042f0&reaction=dislike'>👎</a>
--
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]
