vincbeck commented on code in PR #65685:
URL: https://github.com/apache/airflow/pull/65685#discussion_r3126449531
##########
providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py:
##########
@@ -300,11 +300,61 @@ def is_logged_in(self) -> bool:
"""Return whether the user is logged in."""
user = self.get_user()
return bool(
- self.appbuilder
- and self.appbuilder.app.config.get("AUTH_ROLE_PUBLIC", None)
- or (not user.is_anonymous and user.is_active)
+ (self.appbuilder and self._get_auth_role_public()) or (not
user.is_anonymous and user.is_active)
)
+ def _get_auth_role_public(self) -> str | None:
+ """
+ Return the role granted to anonymous users, or ``None`` if public
access is disabled.
+
+ Prefers the Airflow ``[fab] auth_role_public`` configuration and falls
back to the legacy
+ ``AUTH_ROLE_PUBLIC`` entry in ``webserver_config.py`` so existing
deployments continue to
+ work.
+ """
+ role = conf.get("fab", "auth_role_public", fallback="") or ""
+ if role:
+ return role
+ if self.appbuilder is not None:
+ return self.appbuilder.app.config.get("AUTH_ROLE_PUBLIC", None)
+ return None
+
+ def build_public_user(self) -> AnonymousUser | None:
+ """
+ Build an :class:`AnonymousUser` pre-populated with the configured
public role.
+
+ Returns ``None`` when ``[fab] auth_role_public`` (or the legacy
+ ``AUTH_ROLE_PUBLIC`` entry in ``webserver_config.py``) is not set.
+
+ :meta private:
+ """
+ public_role_name = self._get_auth_role_public()
+ if not public_role_name:
+ return None
+
+ user = AnonymousUser()
+ flask_app = self.flask_app or (self.appbuilder.app if self.appbuilder
else None)
+ if flask_app is not None:
+ with flask_app.app_context():
+ flask_app.config["AUTH_ROLE_PUBLIC"] = public_role_name
+ role = flask_app.appbuilder.sm.find_role(public_role_name)
+ if role is not None:
+ # ``AnonymousUser.roles`` is a lazy property that calls
+ # ``security_manager.get_public_role()`` on every access,
which needs a Flask
+ # request context we do not have under FastAPI. Writing
``_roles``/``_perms``
+ # directly freezes a snapshot of the public role's
permissions for the
+ # lifetime of a single FastAPI authorization check (see
#60897).
+ user._roles = {role}
+ user._perms = {(perm.action.name, perm.resource.name) for
perm in role.permissions}
Review Comment:
Instead of doing this, I think it would be simpler to create a User object
by and (like simple auth manager). Ideally I do not want to create a Flask
context in this file
##########
providers/fab/newsfragments/65685.bugfix.rst:
##########
@@ -0,0 +1 @@
+Honor ``AUTH_ROLE_PUBLIC`` in the FastAPI-based API server. Add a new ``[fab]
auth_role_public`` Airflow configuration that is used consistently by both the
FastAPI and legacy Flask FAB auth stacks so that setting a public role actually
removes the login prompt. Setting ``AUTH_ROLE_PUBLIC`` in
``webserver_config.py`` keeps working for backwards compatibility.
Review Comment:
No need
##########
providers/fab/src/airflow/providers/fab/auth_manager/fab_auth_manager.py:
##########
@@ -300,11 +300,61 @@ def is_logged_in(self) -> bool:
"""Return whether the user is logged in."""
user = self.get_user()
return bool(
- self.appbuilder
- and self.appbuilder.app.config.get("AUTH_ROLE_PUBLIC", None)
- or (not user.is_anonymous and user.is_active)
+ (self.appbuilder and self._get_auth_role_public()) or (not
user.is_anonymous and user.is_active)
)
+ def _get_auth_role_public(self) -> str | None:
+ """
+ Return the role granted to anonymous users, or ``None`` if public
access is disabled.
+
+ Prefers the Airflow ``[fab] auth_role_public`` configuration and falls
back to the legacy
+ ``AUTH_ROLE_PUBLIC`` entry in ``webserver_config.py`` so existing
deployments continue to
+ work.
+ """
+ role = conf.get("fab", "auth_role_public", fallback="") or ""
+ if role:
+ return role
+ if self.appbuilder is not None:
+ return self.appbuilder.app.config.get("AUTH_ROLE_PUBLIC", None)
+ return None
+
+ def build_public_user(self) -> AnonymousUser | None:
+ """
+ Build an :class:`AnonymousUser` pre-populated with the configured
public role.
+
+ Returns ``None`` when ``[fab] auth_role_public`` (or the legacy
+ ``AUTH_ROLE_PUBLIC`` entry in ``webserver_config.py``) is not set.
+
+ :meta private:
+ """
+ public_role_name = self._get_auth_role_public()
+ if not public_role_name:
+ return None
+
+ user = AnonymousUser()
+ flask_app = self.flask_app or (self.appbuilder.app if self.appbuilder
else None)
Review Comment:
`self.appbuilder.app` should be the only way to access the app.
`self.flask_app` should be removed
##########
providers/fab/docs/auth-manager/webserver-authentication.rst:
##########
@@ -42,9 +42,27 @@ following CLI commands to create an account:
--role Admin \
--email [email protected]
-To deactivate the authentication and allow users to be identified as
Anonymous, the following entry
-in ``$AIRFLOW_HOME/webserver_config.py`` needs to be set with the desired role
that the Anonymous
-user will have by default:
+To deactivate the authentication and allow users to be identified as
Anonymous, set the
+``[fab] auth_role_public`` Airflow configuration to the desired role that
anonymous users will have
Review Comment:
That is fab specific, it should not be in airflow core documentation
##########
providers/fab/src/airflow/providers/fab/www/app.py:
##########
@@ -84,6 +84,15 @@ def remove_duplicate_date_header(response):
with flask_app.app_context():
flask_app.config.from_pyfile(webserver_config, silent=True)
+ # Bridge ``[fab] auth_role_public`` Airflow config into the Flask app
config so legacy FAB
+ # code paths that read ``AUTH_ROLE_PUBLIC`` from ``current_app.config``
(e.g.
+ # ``AnonymousUser.roles``, basic_auth, kerberos_auth, security manager)
stay in sync with
+ # the FastAPI-based auth flow. The Airflow config takes precedence over
+ # ``webserver_config.py`` when both are set so there is a single source of
truth.
+ auth_role_public_conf = conf.get("fab", "auth_role_public", fallback="")
or ""
+ if auth_role_public_conf:
+ flask_app.config["AUTH_ROLE_PUBLIC"] = auth_role_public_conf
Review Comment:
Nice, since we are doing that, there is no need to check for
`conf.get("fab", "auth_role_public")` in other places?
##########
airflow-core/src/airflow/api_fastapi/core_api/app.py:
##########
@@ -174,6 +175,9 @@ def init_middlewares(app: FastAPI) -> None:
app.add_middleware(SimpleAllAdminMiddleware)
Review Comment:
This then should be removed and simple auth manager should override
`get_fastapi_middlewares`
--
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]
