[GitHub] [superset] dpgaspar commented on a diff in pull request #21931: feat(reports): execute as other than selenium user

[GitBox <gi...@apache.org>]

dpgaspar commented on code in PR #21931:
URL: https://github.com/apache/superset/pull/21931#discussion_r1006960139


##########
superset/config.py:
##########
@@ -1129,6 +1129,14 @@ def EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument
 # sliding cron window size, should be synced with the celery beat config minus 1 second
 ALERT_REPORTS_CRON_WINDOW_SIZE = 59
 ALERT_REPORTS_WORKING_TIME_OUT_KILL = True
+# Which user to attempt to execute Alerts/Reports as. By default,
+# use the user defined in the `THUMBNAIL_SELENIUM_USER` config parameter.
+# To first try to execute as the creator, then fall back to last modifier, owner and

Review Comment:
   nit:
   ```
   To first try to execute as the report creator, then fall back to last modifier, first owner and
   ```



##########
superset/utils/machine_auth.py:
##########
@@ -43,7 +45,7 @@ def __init__(
     def authenticate_webdriver(
         self,
         driver: WebDriver,
-        user: "User",
+        user: User,

Review Comment:
   can you change `__init__` also?



##########
tests/integration_tests/reports/commands_tests.py:
##########
@@ -649,6 +663,65 @@ def test_email_chart_report_schedule(
         assert_log(ReportState.SUCCESS)
 
 
+@pytest.mark.usefixtures(
+    "load_birth_names_dashboard_with_slices", "create_report_email_chart_alpha_owner"
+)
+@patch("superset.reports.notifications.email.send_email_smtp")
+@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
+def test_email_chart_report_schedule_alpha_owner(

Review Comment:
   can you add more tests to assert that if a report does not have an owner, created by will be used for example. You could potentially unittest `_get_user` for example



##########
superset/reports/commands/execute.py:
##########
@@ -77,11 +77,24 @@
 logger = logging.getLogger(__name__)
 
 
-def _get_user() -> User:
-    user = security_manager.find_user(username=app.config["THUMBNAIL_SELENIUM_USER"])
-    if not user:
-        raise ReportScheduleSelleniumUserNotFoundError()
-    return user
+def _get_user(report_schedule: ReportSchedule) -> User:
+    user_types = app.config["ALERT_REPORTS_EXECUTE_AS"]
+    for user_type in user_types:
+        if user_type == "selenium":

Review Comment:
   what do you think about defining an `Enum` for this?



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org