sadpandajoe commented on code in PR #44243:
URL: https://github.com/apache/superset/pull/44243#discussion_r4130031449
##########
docs/admin_docs/configuration/cache.mdx:
##########
@@ -311,21 +311,35 @@ CACHE_WARMUP_EXECUTORS = [FixedExecutor("admin")]
Use a dedicated read-only service account here rather than a personal admin
account, so that
thumbnail rendering and cache warmup tasks don't fail if a specific user's
credentials change.
-Additional Selenium WebDriver configuration can be set using
`WEBDRIVER_CONFIGURATION`. You can
-implement a custom function to authenticate Selenium. The default function
uses the `flask-login`
-session cookie. Here's an example of a custom function signature:
+Thumbnails are rendered with Playwright and a headless Chromium browser. Extra
Chromium launch
+arguments can be set using `WEBDRIVER_OPTION_ARGS`. You can implement a custom
function to
+authenticate the Playwright browser context. The default function uses the
`flask-login` session
+cookie. Here's an example of a custom function signature:
```python
-def auth_driver(driver: WebDriver, user: "User") -> WebDriver:
- pass
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from flask_appbuilder.security.sqla.models import User
+ from playwright.sync_api import BrowserContext
+
+
+def auth_browser_context(browser_context: BrowserContext, user: User) ->
BrowserContext:
+ # Add cookies or headers that authenticate as `user`, then return the
context.
+ return browser_context
Review Comment:
As written this returns `browser_context` unmodified, so setting
`WEBDRIVER_AUTH_FUNC` to it replaces the working session-cookie auth with a
no-op — screenshot requests become unauthenticated instead of authenticated as
`user`. Could the comment make it unmistakable that this line must be replaced
before use, or could the example show one real auth step?
##########
docs/admin_docs/configuration/cache.mdx:
##########
@@ -311,21 +311,35 @@ CACHE_WARMUP_EXECUTORS = [FixedExecutor("admin")]
Use a dedicated read-only service account here rather than a personal admin
account, so that
thumbnail rendering and cache warmup tasks don't fail if a specific user's
credentials change.
-Additional Selenium WebDriver configuration can be set using
`WEBDRIVER_CONFIGURATION`. You can
-implement a custom function to authenticate Selenium. The default function
uses the `flask-login`
-session cookie. Here's an example of a custom function signature:
+Thumbnails are rendered with Playwright and a headless Chromium browser. Extra
Chromium launch
+arguments can be set using `WEBDRIVER_OPTION_ARGS`. You can implement a custom
function to
+authenticate the Playwright browser context. The default function uses the
`flask-login` session
+cookie. Here's an example of a custom function signature:
```python
-def auth_driver(driver: WebDriver, user: "User") -> WebDriver:
- pass
+from __future__ import annotations
Review Comment:
`from __future__ import annotations` has to be the first statement in a
module, so pasting this snippet below other settings in an existing
`superset_config.py` raises a `SyntaxError` at startup for every service, not
just workers. Could the annotations be quoted strings instead
(`browser_context: "BrowserContext"`) so the example doesn't depend on where
it's placed in the file?
##########
docs/admin_docs/configuration/cache.mdx:
##########
@@ -241,7 +241,7 @@ FEATURE_FLAGS = {
}
```
-By default thumbnails are rendered per user, and will fall back to the
Selenium user for anonymous users.
+By default thumbnails are rendered as the user who requests them.
Review Comment:
With the default `THUMBNAIL_EXECUTORS = [ExecutorType.CURRENT_USER]`, an
anonymous request has no user to render as, so `get_executor()` raises instead
of producing a thumbnail — this line reads as if anonymous requests still
succeed. Should this call out the anonymous case explicitly, the way the old
fallback sentence used to?
##########
docs/admin_docs/configuration/cache.mdx:
##########
@@ -311,21 +311,35 @@ CACHE_WARMUP_EXECUTORS = [FixedExecutor("admin")]
Use a dedicated read-only service account here rather than a personal admin
account, so that
thumbnail rendering and cache warmup tasks don't fail if a specific user's
credentials change.
-Additional Selenium WebDriver configuration can be set using
`WEBDRIVER_CONFIGURATION`. You can
-implement a custom function to authenticate Selenium. The default function
uses the `flask-login`
-session cookie. Here's an example of a custom function signature:
+Thumbnails are rendered with Playwright and a headless Chromium browser. Extra
Chromium launch
+arguments can be set using `WEBDRIVER_OPTION_ARGS`. You can implement a custom
function to
+authenticate the Playwright browser context. The default function uses the
`flask-login` session
+cookie. Here's an example of a custom function signature:
```python
-def auth_driver(driver: WebDriver, user: "User") -> WebDriver:
- pass
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from flask_appbuilder.security.sqla.models import User
+ from playwright.sync_api import BrowserContext
+
+
+def auth_browser_context(browser_context: BrowserContext, user: User) ->
BrowserContext:
+ # Add cookies or headers that authenticate as `user`, then return the
context.
+ return browser_context
```
Then on configuration:
```
-WEBDRIVER_AUTH_FUNC = auth_driver
+WEBDRIVER_AUTH_FUNC = auth_browser_context
```
+To replace the authentication logic entirely, subclass
`superset.utils.machine_auth.MachineAuthProvider`,
+override `authenticate_browser_context()`, and set
`MACHINE_AUTH_PROVIDER_CLASS` to your class.
Review Comment:
`MACHINE_AUTH_PROVIDER_CLASS` is loaded via `load_class_from_name`, which
calls `.split(".")` on the config value, so assigning it an actual class object
(as "your class" reads) raises an `AttributeError` at startup instead of
working.
```suggestion
override `authenticate_browser_context()`, and set
`MACHINE_AUTH_PROVIDER_CLASS` to the fully-qualified dotted path of your class
(for example, `"myapp.auth.MyMachineAuthProvider"`).
```
--
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]
