john-bodley commented on code in PR #20502:
URL: https://github.com/apache/superset/pull/20502#discussion_r906709592
##########
superset/tasks/async_queries.py:
##########
@@ -76,30 +67,35 @@ def load_chart_data_into_cache(
# pylint: disable=import-outside-toplevel
from superset.charts.data.commands.get_data_command import ChartDataCommand
- try:
- ensure_user_is_set(job_metadata.get("user_id"))
Review Comment:
I'm not certain whether this logic was correct, i.e., should we ensure that
`job_metadata.get("user_id")` is the user performing said actions regardless of
whether there is a current user set? Note the `override_user` context manager
will reset back to the current user upon exit.
@benjreinhart it seems you authored
https://github.com/apache/superset/pull/13878 and thus I was hoping you might
be able to provide more context, i.e., on line #75 should it be
`override_user(user)` rather than `overide_user(user, force=False)`?
##########
superset/utils/core.py:
##########
@@ -1453,23 +1453,27 @@ def get_user_id() -> Optional[int]:
@contextmanager
-def override_user(user: Optional[User]) -> Iterator[Any]:
+def override_user(user: Optional[User], force: bool = True) -> Iterator[Any]:
Review Comment:
The `force` flag was added for parity, however per my previous comment I'm
not sure it's needed.
##########
tests/integration_tests/access_tests.py:
##########
@@ -562,34 +562,34 @@ def test_get_username(
assert get_username() == username
[email protected](
- "username",
- [
- None,
- "alpha",
- "gamma",
- ],
-)
[email protected]("username", [None, "alpha", "gamma"])
[email protected]("force", [False, True])
def test_override_user(
app_context: AppContext,
mocker: MockFixture,
username: str,
+ force: bool,
) -> None:
mock_g = mocker.patch("superset.utils.core.g", spec={})
admin = security_manager.find_user(username="admin")
user = security_manager.find_user(username)
+ with override_user(user, force):
+ assert mock_g.user == user
+
assert not hasattr(mock_g, "user")
- with override_user(user):
+ mock_g.user = None
Review Comment:
Additional tests.
--
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]
