Airflow graceful loading for plugins for users without read permissions #51714
Conversation
Co-authored-by: Pierre Jeambrun <pierrejbrun@gmail.com>
tanujdargan
requested review from
bbovenzi,
bugraoz93,
ephraimbuddy,
jason810496,
jscheffl,
pierrejeambrun,
rawwar,
ryanahamilton and
shubhamraj-git
as code owners
boring-cyborg
bot
added
area:API
|
@pierrejeambrun I had to create a new PR as I rebased by mistake while shifting between dev environments but this one shouldn't require anything to be done hopefully. |
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/config.py
Outdated
Show resolved
Hide resolved
|
@tanujdargan , I created a new user with the "Viewer" role, and when I log in, I still see 403 errors with your changes:
|
Refactored config.py for ui api routes. Fixed 403 error still showing for plugin imports
|
@rawwar You were right about the 403 error still being there I've gone ahead and pushed an update for that, should be all good now! |
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/config.py
Outdated
Show resolved
Hide resolved
|
@pierrejeambrun Could you review? |
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
Thanks, looking good, just a few suggestions / questions.
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/config.py
Outdated
Show resolved
Hide resolved
| const { data, error, isLoading } = usePluginServiceImportErrors(undefined, { | ||
| retry: false, | ||
| }); |
There was a problem hiding this comment.
This is a deliberate optimization. It prevents the UI from making multiple, pointless API calls when a user gets a 403 (Forbidden) error, as their permissions are unlikely to change between retries.
There was a problem hiding this comment.
I don't think we should do that. Request could fail for many different reason. (network error because of poor connection, in the subway for instance, or other status code) and should be retried.
This is consistent with the rest of the application. Indeed if someone does not have permission, request will be sent two times for no reason, but that's where we are at the moment.
That needs to be solved at the entire application level and not only in one particular API call.
I suggest to remove this and follow up with a dedicated PR if you have ideas on how to approach this.
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_config.py
Outdated
Show resolved
Hide resolved
…nfig.py Co-authored-by: Pierre Jeambrun <pierrejbrun@gmail.com>
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
1 small nit, working as expected, thanks for the PR.
| const { data, error, isLoading } = usePluginServiceImportErrors(undefined, { | ||
| retry: false, | ||
| }); |
There was a problem hiding this comment.
I don't think we should do that. Request could fail for many different reason. (network error because of poor connection, in the subway for instance, or other status code) and should be retried.
This is consistent with the rest of the application. Indeed if someone does not have permission, request will be sent two times for no reason, but that's where we are at the moment.
That needs to be solved at the entire application level and not only in one particular API call.
I suggest to remove this and follow up with a dedicated PR if you have ideas on how to approach this.
There was a problem hiding this comment.
This need some small adjustments, following #51889 merge.
appbuilder_menu_items is deprecated and we should instead return external_views that target the None or "nav" destination. (both will endup being extra menu items). Or you can return all external_views and do the filtering in the UI if "destination==="nav" (None does not need to be checked there because the API fills None with "nav" when returning a response).
| dashboard_alert: list[UIAlert] | ||
| show_external_log_redirect: bool | ||
| external_log_name: str | None = None | ||
| plugins_extra_menu_items: list[AppBuilderMenuItemResponse] = [] |
There was a problem hiding this comment.
You don't need to provide a default. This can never be None and will be passed from the plugin manager.
|
It makes more sense to start this from scratch since most of the stuff would need to be redone and I would have a lot of unnecessary conflicts. |
|
@tanujdargan Thanks for you message, I'm not sure starting over would be the fastest way, do you plan to open a new PR with the updates? |
|
I partially started over, it was more efficient since I had a lot of merge conflicts some of which couldn't be resolved. I've raised a new PR here: #52408 |
3 participants
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
routes/ui/config.pyto safely handle cases whereplugins_manager.flask_appbuilder_menu_linksisNone, preventing 403 errors from blocking the UI.plugins_extra_menu_itemsattribute in the UI config endpoint, so all authenticated users can see them, regardless of direct plugin permissions.public/providersendpoint for these menu items.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.