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

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: dev@airflow.apache.org
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

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

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 :)

pierrejeambrun

LGTM

vincbeck

2 nits but overall looks very good!

airflow-core/docs/core-concepts/auth-manager/index.rst

airflow-core/newsfragments/59399.feature.rst

airflow-core/docs/core-concepts/auth-manager/index.rst

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!

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

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 <97131062+vincbeck@users.noreply.github.com>

…m authorization logic for HITL tasks

jason810496

Thanks for the update! LGTM if CI pass.

I just added backport label to 3.1.6, we can close the backport one if we only want to release in 3.2.0.

boring-cyborg · 2026-01-05T09:21:17Z

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

github-actions · 2026-01-05T09:22:37Z

Backport failed to create: v3-1-test. View the failure log Run details

Status	Branch	Result
❌	v3-1-test

You can attempt to backport this manually by running:

cherry_picker d95d8ba v3-1-test

This should apply the commit to the v3-1-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

…ns=True (apache#59399) (cherry picked from commit d95d8ba) Co-authored-by: Victor Kwong <109138344+TempestShaw@users.noreply.github.com> Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>

…ns=True (#59399) (#60116) Co-authored-by: Victor Kwong <109138344+TempestShaw@users.noreply.github.com> Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>

…che#59399) Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>

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

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

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

pierrejeambrun approved these changes Dec 16, 2025

View reviewed changes

vincbeck reviewed Dec 16, 2025

View reviewed changes

airflow-core/docs/core-concepts/auth-manager/index.rst Outdated Show resolved Hide resolved

airflow-core/newsfragments/59399.feature.rst Outdated Show resolved Hide resolved

uranusjr reviewed Dec 22, 2025

View reviewed changes

airflow-core/docs/core-concepts/auth-manager/index.rst Show resolved Hide resolved

jason810496 reviewed Dec 24, 2025

View reviewed changes

TempestShaw marked this pull request as draft January 4, 2026 09:49

TempestShaw and others added 9 commits January 4, 2026 22:41

Refactor HITL authorization methods across auth managers to improve c…

4c923aa

…onsistency and remove redundant code

Refactor is_authorized_hitl_task signature

535aa03

- 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

Refactor HITL authorization methods across auth managers to improve c…

e058625

…onsistency and remove redundant code

Refactor is_authorized_hitl_task signature

6df3d61

- 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

659abfd

Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>

Clarify is_authorized_hitl_task method documentation to specify custo…

216e684

…m authorization logic for HITL tasks

modify documentation for is_authorized_hitl_task method

dbc6f74

TempestShaw force-pushed the fix-hitl-auth branch from 65f58ed to dbc6f74 Compare January 4, 2026 15:44

TempestShaw marked this pull request as ready for review January 4, 2026 15:46

jason810496 approved these changes Jan 5, 2026

View reviewed changes

jason810496 added the backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch label Jan 5, 2026

vatsrahul1001 approved these changes Jan 5, 2026

View reviewed changes

jason810496 closed this Jan 5, 2026

jason810496 reopened this Jan 5, 2026

Lee-W approved these changes Jan 5, 2026

View reviewed changes

Lee-W merged commit d95d8ba into apache:main Jan 5, 2026
274 of 282 checks passed

Lee-W mentioned this pull request Jan 5, 2026

[v3-1-test] Fix ApprovalOperator with SimpleAuthManager when all_admi… #60116

Merged

chirodip98 pushed a commit to chirodip98/airflow-contrib that referenced this pull request Jan 8, 2026

Fix ApprovalOperator with SimpleAuthManager when all_admins=True (apa…

a99a973

…che#59399) Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>

chirodip98 pushed a commit to chirodip98/airflow-contrib that referenced this pull request Jan 9, 2026

Fix ApprovalOperator with SimpleAuthManager when all_admins=True (apa…

e092d00

…che#59399) Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>

stegololz pushed a commit to stegololz/airflow that referenced this pull request Jan 9, 2026

Fix ApprovalOperator with SimpleAuthManager when all_admins=True (apa…

da50c2f

…che#59399) Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>

Fix ApprovalOperator with SimpleAuthManager when all_admins=True #59399

Fix ApprovalOperator with SimpleAuthManager when all_admins=True #59399

Uh oh!

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!

pierrejeambrun left a comment

Choose a reason for hiding this comment

Uh oh!

vincbeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TempestShaw commented Dec 24, 2025

Uh oh!

jason810496 left a comment

Choose a reason for hiding this comment

Uh oh!

jason810496 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

boring-cyborg bot commented Jan 5, 2026

Uh oh!

github-actions bot commented Jan 5, 2026

Backport failed to create: v3-1-test. View the failure log Run details

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants