Airflow graceful loading for plugins for users without read permissions #52408

tanujdargan · 2025-06-28T13:01:32Z

Fixes: #51299 - Impossible to load UI if user does not have read permissions on plugins

Summary

This PR ensures the Airflow UI loads gracefully for authenticated users who do not have read permissions on plugins.

Details

Updated approach to work following Adjustment external views #51889 merge.
Refactored routes/ui/config.py to safely handle cases where plugins_manager.flask_appbuilder_menu_links is None, preventing 403 errors from blocking the UI.
Adjusted import order for clarity and maintainability.
The UI now loads extra plugin menu items from a new plugins_extra_menu_items attribute in the UI config endpoint, so all authenticated users can see them, regardless of direct plugin permissions.
The UI no longer calls the public/providers endpoint for these menu items.
Fixes failing CI pipeline and other tests.

Context

Previously, users without read permissions on plugins would receive a 403 Forbidden error, causing the entire UI to fail rendering. With this fix, the UI continues to render and display default elements, and plugin menu items are provided in a way accessible to all authenticated users.

Closes #51299.

jason810496

Nice! Thanks for raising the PR.

jason810496 · 2025-06-29T14:31:49Z

airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py

    dependencies=[Depends(requires_authenticated())],
 )
-def get_configs() -> ConfigResponse:
+async def get_configs(user: GetUserDep) -> ConfigResponse:


Why do we need to change this router to async ?

get_config depends on GetUserDep which is async and as per FastAPI rule: any route function that calls an async dependency must also be async.

and, the previous implementation allowed any authenticated user to see all UI plugins, which was a security issue. My PR fixes this by correctly checking the user's permissions for each plugin before displaying it. While the default Viewer role in the documentation is shown to have plugin access, my change ensures the system properly enforces whatever permissions are actually configured. So, if a Viewer has no plugin permissions, they will no longer see any plugin menu items."

get_config depends on GetUserDep which is async and as per FastAPI rule: any route function that calls an async dependency must also be async.

Where do you get this information from ? I tend to think that it does not matter and this is confirmed by FastAPI documentation:
https://fastapi.tiangolo.com/tutorial/dependencies/#to-async-or-not-to-async

jason810496 · 2025-06-29T14:37:36Z

airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py

+    auth_manager = get_auth_manager()
+    authorized_plugins = []
+    if plugins_manager.external_views:
+        for view in plugins_manager.external_views:
+            if auth_manager.is_authorized_custom_view(method="GET", resource_name=view["name"], user=user):
+                authorized_plugins.append(view)


Do we need to doc the #51714 (review) comment here to describe the logic ?

Yup f41d1c9

I think we are mixing things up. Why would plugin permissions be in the auth manger custom views permissions? Those are not related, permissions to access plugins is AccessView.PLUGINS.

Also we assume that this subpart of the plugins (external views in the nav) are public, so everybody can load the UI or access it.

jason810496 · 2025-06-29T14:41:07Z

airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_config.py

 from tests_common.test_utils.config import conf_vars

-pytestmark = pytest.mark.db_test
+pytestmark = [pytest.mark.db_test, pytest.mark.mock_plugin_manager(plugins=[])]


How about we don't mock the plugin manager return an empty list. Let's make the external_views return some views that belongs to nav and some are not belongs to nav to ensure the implementaion in router.

I agree, just pushed a fix for this! f41d1c9

bbovenzi · 2025-06-30T18:54:04Z

airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenus.tsx

-  // Only show external plugins in menu if there are more than 2
-  const menuExternalViews = menuPlugins.length > 2 ? menuPlugins : [];
-  const directExternalViews = menuPlugins.length <= 2 ? menuPlugins : [];


Did we lose the code to consolidate plugin views into a menu when there are many of them?

The current logic does seem to replace the more granular is_menu_item flag-based separation with a hard cutoff at 2 items:

This effectively overrides the earlier logic that distinguishes between menuExternalViews and directExternalViews using the is_menu_item property. If we previously consolidated plugins into a dropdown based on that flag (rather than count), this could be a regression or unintentional change.

Should I revert to using is_menu_item to preserve expected grouping behaviour?

Yeah, this PR shouldn't change how we actually show plugins in the nav bar.

pierrejeambrun

A few suggestions / comments.

pierrejeambrun · 2025-07-01T08:50:18Z

airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py

    config = {key: conf.get("api", key) for key in API_CONFIG_KEYS}

    task_log_reader = TaskLogReader()
+    plugins_manager.initialize_ui_plugins()


You do not need to do that, this is done at the init of the FastAPI app, can you remove it?

pierrejeambrun · 2025-07-01T08:51:39Z

airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py

    dependencies=[Depends(requires_authenticated())],
 )
-def get_configs() -> ConfigResponse:
+async def get_configs(user: GetUserDep) -> ConfigResponse:


get_config depends on GetUserDep which is async and as per FastAPI rule: any route function that calls an async dependency must also be async.

Where do you get this information from ? I tend to think that it does not matter and this is confirmed by FastAPI documentation:
https://fastapi.tiangolo.com/tutorial/dependencies/#to-async-or-not-to-async

pierrejeambrun · 2025-07-01T08:54:46Z

airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py

+    auth_manager = get_auth_manager()
+    authorized_plugins = []
+    if plugins_manager.external_views:
+        for view in plugins_manager.external_views:
+            if auth_manager.is_authorized_custom_view(method="GET", resource_name=view["name"], user=user):
+                authorized_plugins.append(view)


I think we are mixing things up. Why would plugin permissions be in the auth manger custom views permissions? Those are not related, permissions to access plugins is AccessView.PLUGINS.

Also we assume that this subpart of the plugins (external views in the nav) are public, so everybody can load the UI or access it.

pierrejeambrun · 2025-07-01T08:55:39Z

airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py

+
+    # First, we check which of the available external plugin views the current user is authorized to see.
+    auth_manager = get_auth_manager()
+    authorized_plugins = []


I think we should rename this, those are external_views, not plugins.

pierrejeambrun · 2025-07-01T08:57:01Z

airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenus.tsx

+  const menuPlugins = (useConfig("plugins_extra_menu_items") as Array<ExternalViewResponse>) ?? [];



We shouldn't need a hard casting like that. "as"

pierrejeambrun · 2025-07-01T08:58:58Z

airflow-core/src/airflow/ui/src/layouts/Nav/PluginMenus.tsx

+  const directExternalViews = menuPlugins.filter((p: ExternalViewResponse) => !p.is_menu_item);
+  const menuExternalViews = menuPlugins.filter((p: ExternalViewResponse) => p.is_menu_item);


I don't understand what's going on here. What is is_menu_item attribute, that does not exist.

pierrejeambrun · 2025-07-01T09:00:13Z

airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_config.py


-class TestGetConfig:
+@pytest.mark.mock_plugin_manager(plugins=[])
+class TestGetConfigWithNoPlugins:


1 class per endpoint. Multiple method for different test cases. We shouldn't have two classes there.

pierrejeambrun · 2025-07-01T09:05:10Z

Also, just an idea, but maybe we can handle that simply in the UI. Call the endoint if 403 forbidden -> instead of crashing everything, catch the error and return an empty plugin list, then the UI will proceed ignoring the plugins.

Because mixing up the plugin and the config do not feel super clean

bbovenzi

+1 to Pierre's comment to keep the plugins endpoint and just handle a 403 better.

Marking as changes requested because there is a lot of hallucinated AI code in this PR that needs to be corrected. Please review your AI generated code before opening a PR. Otherwise it forces code reviewers to do your work for you.

tanujdargan requested review from bbovenzi, bugraoz93, ephraimbuddy, jason810496, jscheffl, pierrejeambrun, rawwar, ryanahamilton and shubhamraj-git as code owners June 28, 2025 13:01

boring-cyborg bot added area:API Airflow's REST/HTTP API area:UI Related to UI/UX. For Frontend Developers. labels Jun 28, 2025

tanujdargan mentioned this pull request Jun 28, 2025

Airflow graceful loading for plugins for users without read permissions #51714

Closed

jason810496 reviewed Jun 29, 2025

View reviewed changes

bbovenzi reviewed Jun 30, 2025

View reviewed changes

pierrejeambrun reviewed Jul 1, 2025

View reviewed changes

bbovenzi requested changes Jul 1, 2025

View reviewed changes

tanujdargan closed this Jul 2, 2025

tanujdargan force-pushed the main branch from f41d1c9 to 4c780cc Compare July 2, 2025 17:31

pierrejeambrun mentioned this pull request Aug 7, 2025

Fixes loading UI if user does not have read permissions on plugins #54113

Closed

		const menuPlugins = (useConfig("plugins_extra_menu_items") as Array<ExternalViewResponse>) ?? [];

		const directExternalViews = menuPlugins.filter((p: ExternalViewResponse) => !p.is_menu_item);
		const menuExternalViews = menuPlugins.filter((p: ExternalViewResponse) => p.is_menu_item);

Airflow graceful loading for plugins for users without read permissions #52408

Airflow graceful loading for plugins for users without read permissions #52408

Uh oh!

Conversation

tanujdargan commented Jun 28, 2025

Fixes: #51299 - Impossible to load UI if user does not have read permissions on plugins

Summary

Details

Context

Uh oh!

jason810496 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierrejeambrun Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierrejeambrun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierrejeambrun Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierrejeambrun commented Jul 1, 2025

Uh oh!

bbovenzi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pierrejeambrun Jul 1, 2025 •

edited

Loading

pierrejeambrun Jul 1, 2025 •

edited

Loading

bbovenzi left a comment •

edited

Loading