Fix ApprovalOperator with SimpleAuthManager when all_admins=True #59399

TempestShaw · 2025-12-14T04:24:57Z

Fixes #59348. Added is_allowed() method to BaseAuthManager and all
implementations to properly delegate HITL permission checks.

SimpleAuthManager: Returns True when simple_auth_manager_all_admins=True
Other managers: Check if user is in assigned_users list
Updated hitl.py to use auth manager's is_allowed() method

Fixes apache#59348. Added is_allowed() method to BaseAuthManager and all implementations to properly delegate HITL permission checks. - SimpleAuthManager: Returns True when simple_auth_manager_all_admins=True - Other managers: Check if user is in assigned_users list - Updated hitl.py to use auth manager's is_allowed() method Remove duplicate import

boring-cyborg · 2025-12-14T04:25:06Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

jason810496

Nice! Thanks for the fix and it LGTM overall.

airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py

providers/amazon/src/airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py

…onsistency and remove redundant code

jason810496

Thanks for the update!

Also that is a new functionality and some thinking on how we communicatee it (newsfragment) and what should be default behaviour for Auth Managers that do not implement it should be.

It would be nice to add the 59399.feature.rst newsfragment from airflow-core/newsfragments/template.significant.rst template.

Thanks!

airflow-core/src/airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py

TempestShaw · 2025-12-15T14:55:44Z

Thanks, will do after a long haul flight.

airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py

airflow-core/src/airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py

airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py

vincbeck · 2025-12-15T16:17:10Z

Some comments but the overall direction is good I think

vincbeck · 2025-12-15T16:19:18Z

Please also update documentation to mention this new API in Authorization related methods section

- Update method to use keyword-only parameters - Take full user object instead of just user_id - Add unit tests for BaseAuthManager and SimpleAuthManager - Update hitl.py call site to match new signature

Lee-W

overall looks good. But would like to confirm with @vincbeck whether it's possible to have more than one user with the same ID (I guess not?) if so, should we use id, name pair to check instead

potiuk · 2025-12-16T11:55:36Z

overall looks good. But would like to confirm with @vincbeck whether it's possible to have more than one user with the same ID (I guess not?) if so, should we use id, name pair to check instead

Nope. you can't. User_id uniquely identifies user in AuthManager.

potiuk · 2025-12-16T11:57:16Z

Looks way better now :)

…ead to Internal Server Error in API server (apache#59382) * refactor: Fix logout route in Keycloak provider also so the KeycloakPostError doesn't propagate to API server also which leads to Internal Server Error * refactor: Fixed static checks * refactor: Fixed refresh_token invocations * refactor: Must call refresh_user in refresh route * refactor: refresh_token must always return a dict * refactor: Added test when keycloak client raises KeycloakPostError when refresh_token is being invoked in logout route * refactor: Fixed some additional static checks * refactor: Refactored refresh_user * refactor: Reformatted imports * refactor: Fixed mocking in refresh test * refactor: Removed unused mocking of keycloak client in test_refresh_token * refactor: Fixed mock get_auth_manager and added missing import KeycloakAuthManagerUser * refactor: Refresh token route calls refresh_user instead of refresh_token * refactor: Changed assert on refresh user * Update providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py Co-authored-by: Vincent <[email protected]> * refactor: Fixed calls to refresh_tokens instead of refresh_token --------- Co-authored-by: Vincent <[email protected]>

…#59489) Co-authored-by: nhuan.bc <[email protected]>

…che#59493) This change is needed for compatibility (i.e. stop the test from failing) with the `SQLALCHEMY_ENGINE_DEBUG` flag.

…on (apache#41706) (apache#58841) * Fix AsyncKubernetesHook when Kubernetes connection is missing (apache#41706) * fix CI error * fix CI error apache#2 * Rename `conn_extras` to `connection_extras` for consistency across Kubernetes hooks and operators * Handle AirflowNotFoundException when resolving connection extras in KubernetesPodOperator

Fixes apache#59348. Added is_allowed() method to BaseAuthManager and all implementations to properly delegate HITL permission checks. - SimpleAuthManager: Returns True when simple_auth_manager_all_admins=True - Other managers: Check if user is in assigned_users list - Updated hitl.py to use auth manager's is_allowed() method Remove duplicate import

…onsistency and remove redundant code

- Update method to use keyword-only parameters - Take full user object instead of just user_id - Add unit tests for BaseAuthManager and SimpleAuthManager - Update hitl.py call site to match new signature

Co-authored-by: Vincent <[email protected]>

…m authorization logic for HITL tasks

TempestShaw · 2025-12-24T02:00:01Z

Hey @TempestShaw , I think we're close to merging this one. Could you please help us resolve the remaining comments and conflicts? Thanks!

@Lee-W Hi! I rebased fix-hitl-auth onto main to keep it up to date, which automatically triggered additional review requests.
Just wanted to check if this rebase looks correct. Thanks!

jason810496

@Lee-W Hi! I rebased fix-hitl-auth onto main to keep it up to date, which automatically triggered additional review requests.
Just wanted to check if this rebase looks correct. Thanks!

Thanks for the rebase. We still need to resolve the conflicts again. Thanks!

TempestShaw requested review from bugraoz93, ephraimbuddy, jason810496, o-nikolas, pierrejeambrun, rawwar, shubhamraj-git and vincbeck as code owners December 14, 2025 04:24

boring-cyborg bot added area:API Airflow's REST/HTTP API area:providers provider:amazon AWS/Amazon - related issues provider:fab provider:keycloak labels Dec 14, 2025

jason810496 reviewed Dec 14, 2025

View reviewed changes

airflow-core/src/airflow/api_fastapi/auth/managers/base_auth_manager.py Outdated Show resolved Hide resolved

providers/amazon/src/airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py Outdated Show resolved Hide resolved

jason810496 requested a review from Lee-W December 14, 2025 10:20

Refactor HITL authorization methods across auth managers to improve c…

083c502

…onsistency and remove redundant code

jason810496 reviewed Dec 15, 2025

View reviewed changes

airflow-core/src/airflow/api_fastapi/auth/managers/simple/simple_auth_manager.py Outdated Show resolved Hide resolved

vincbeck reviewed Dec 15, 2025

View reviewed changes

Refactor is_authorized_hitl_task signature

af780b0

- Update method to use keyword-only parameters - Take full user object instead of just user_id - Add unit tests for BaseAuthManager and SimpleAuthManager - Update hitl.py call site to match new signature

Lee-W reviewed Dec 16, 2025

View reviewed changes

potiuk approved these changes Dec 16, 2025

View reviewed changes

Lee-W approved these changes Dec 16, 2025

View reviewed changes

potiuk force-pushed the fix-hitl-auth branch from af780b0 to 11af683 Compare December 16, 2025 16:01

dabla and others added 12 commits December 24, 2025 09:29

Fix message of _read_from_logs_server when status_code is 403 (apache…

ff2cd96

…#59489) Co-authored-by: nhuan.bc <[email protected]>

scheduler test: filter captured messages to skip SQLA debug logs (apa…

4f53a80

…che#59493) This change is needed for compatibility (i.e. stop the test from failing) with the `SQLALCHEMY_ENGINE_DEBUG` flag.

Fix Theme Configs (apache#59473)

b79d083

Add translation for version compare selection placeholder (apache#59346)

f4c6854

Refactor HITL authorization methods across auth managers to improve c…

5c9e85c

…onsistency and remove redundant code

Refactor is_authorized_hitl_task signature

2a0aa68

- Update method to use keyword-only parameters - Take full user object instead of just user_id - Add unit tests for BaseAuthManager and SimpleAuthManager - Update hitl.py call site to match new signature

Update airflow-core/newsfragments/59399.feature.rst

6e4744f

Co-authored-by: Vincent <[email protected]>

Clarify is_authorized_hitl_task method documentation to specify custo…

17d3bb7

…m authorization logic for HITL tasks

modify documentation for is_authorized_hitl_task method

65f58ed

TempestShaw force-pushed the fix-hitl-auth branch from 7267732 to 65f58ed Compare December 24, 2025 01:48

TempestShaw requested review from XD-DENG, amoghrajesh, ashb, bbovenzi, dstandish, gopidesupavan, guan404ming, hussein-awala, jedcunningham, jscheffl, kaxil, mobuchowski, ryanahamilton, sekikn and vatsrahul1001 as code owners December 24, 2025 01:48

jason810496 reviewed Dec 24, 2025

View reviewed changes

Fix ApprovalOperator with SimpleAuthManager when all_admins=True #59399

Are you sure you want to change the base?

Fix ApprovalOperator with SimpleAuthManager when all_admins=True #59399

Conversation

TempestShaw commented Dec 14, 2025

Uh oh!

boring-cyborg bot commented Dec 14, 2025

Uh oh!

jason810496 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jason810496 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TempestShaw commented Dec 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vincbeck commented Dec 15, 2025

Uh oh!

vincbeck commented Dec 15, 2025

Uh oh!

Lee-W left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk commented Dec 16, 2025

Uh oh!

potiuk commented Dec 16, 2025

Uh oh!

TempestShaw commented Dec 24, 2025

Uh oh!

jason810496 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants