Fix: Clean up FAB permissions when deleting DAGs #54528

HsiuChuanHsu · 2025-08-14T23:01:00Z

Description

When a DAG is deleted, the corresponding Flask-AppBuilder (FAB) permissions remain in the database tables (ab_view_menu, ab_permission_view, ab_permission_view_role). This causes

Orphaned permissions: Deleted DAGs continue to appear in the permission management UI
User confusion: Users see permissions for non-existent DAGs

How this happend?

The delete_dag() function in delete_dag.py successfully removes DAG-related records from core Airflow tables but does not clean up the Flask-AppBuilder permission system.

Solution

FAB Provider

dag_permissions.py: New module that handles FAB permission cleanup ( code referenced from

airflow/devel-common/src/tests_common/test_utils/db.py

Lines 358 to 393 in e00777a

    
           def clear_dag_specific_permissions(): 
        
               if "FabAuthManager" not in conf.get("core", "auth_manager"): 
        
                   return 
        
               try: 
        
                   from airflow.providers.fab.auth_manager.models import Permission, Resource, assoc_permission_role 
        
               except ImportError: 
        
                   # Handle Pre-airflow 2.9 case where FAB was part of the core airflow 
        
                   from airflow.providers.fab.auth.managers.fab.models import ( 
        
                       Permission, 
        
                       Resource, 
        
                       assoc_permission_role, 
        
                   ) 
        
               except RuntimeError as e: 
        
                   # Handle case where FAB provider is not even usable 
        
                   if "needs Apache Airflow 2.9.0" in str(e): 
        
                       from airflow.providers.fab.auth.managers.fab.models import ( 
        
                           Permission, 
        
                           Resource, 
        
                           assoc_permission_role, 
        
                       ) 
        
                   else: 
        
                       raise 
        
               with create_session() as session: 
        
                   dag_resources = session.query(Resource).filter(Resource.name.like(f"{RESOURCE_DAG_PREFIX}%")).all() 
        
                   dag_resource_ids = [d.id for d in dag_resources] 
        
                   dag_permissions = session.query(Permission).filter(Permission.resource_id.in_(dag_resource_ids)).all() 
        
                   dag_permission_ids = [d.id for d in dag_permissions] 
        
                   session.query(assoc_permission_role).filter( 
        
                       assoc_permission_role.c.permission_view_id.in_(dag_permission_ids) 
        
                   ).delete(synchronize_session=False) 
        
                   session.query(Permission).filter(Permission.resource_id.in_(dag_resource_ids)).delete( 
        
                       synchronize_session=False 
        
                   ) 
        
                   session.query(Resource).filter(Resource.id.in_(dag_resource_ids)).delete(synchronize_session=False)

)

fab_auth_manager.py: Added cleanup_dag_permissions() method
Tests: New tests for the permission cleanup logic

Core Airflow (delete_dag.py)
- Now calls auth_manager.cleanup_dag_permissions()
- Added error handling so DAG deletion doesn't fail if permission cleanup fails

Replace automatic DAG permission cleanup with CLI command

Changes:
- Remove automatic permission cleanup from delete_dag() function
- Add new airflow fab-auth-manager permissions-cleanup CLI command
- Support both global orphaned permission cleanup and specific DAG cleanup
CLI command supports
- Cleaning up orphaned permissions for deleted DAGs (default behavior)
- Cleaning up permissions for specific DAG with --dag-id option
- Dry-run mode with --dry-run flag
- Skip confirmation with --yes flag
- Verbose output with --verbose flag
Examples:
- airflow fab-auth-manager permissions-cleanup
- airflow fab-auth-manager permissions-cleanup --dry-run
- airflow fab-auth-manager permissions-cleanup --dag-id my_dag
- airflow fab-auth-manager permissions-cleanup --yes

closes: #50905

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

potiuk · 2025-08-18T23:50:09Z

Can you please add some unit tests?

HsiuChuanHsu · 2025-08-19T23:21:57Z

@potiuk, tests are added!

pierrejeambrun

Looking good, just a suggestion because I don't think this code should live in core.

cc: @vincbeck

airflow-core/src/airflow/api/common/delete_dag.py

vincbeck

I am not against this change but here are some thoughts:

Do we really want that? Example: As an admin, I give read permissions for the Dag secret to X users. This Dag gets deleted and re-created, all permissions will be lost and I (the admin) needs to assign again permissions to my users.
Since this is very auth manager specific, and actually very Fab Auth manager specific. Should we not, instead, provide a CLI like airflow fab-auth-manager permissions-cleanup which would delete all orphaned permissions. That way we do not need to wire the mechanism in Airflow with something like if hasattr(...), and it would be up to the admin to run this command when they think it is needed

airflow-core/tests/unit/api/common/test_delete_dag.py

HsiuChuanHsu · 2025-08-26T23:26:16Z

Thanks for the feedbak! I like the idea of using CLI command for it. 💪
It gives admins control over when to clean up orphaned permissions, which I believe is more robust. Plus, it keeps the main code cleaner and less tied to a specific auth manager.

@potiuk What are your thoughts on this? Any downsides I'm not seeing?

potiuk · 2025-08-27T07:31:02Z

@potiuk What are your thoughts on this? Any downsides I'm not seeing?

The asseesment of @vincbeck and @pierrejeambrun are more than enough :) . I like it's off-loaded to Fab Auth Manager, this is FAB specific and having separate cleanup command looks like a good idea.

airflow-core/src/airflow/api/common/delete_dag.py

HsiuChuanHsu · 2025-08-28T23:10:44Z

The asseesment of @vincbeck and @pierrejeambrun are more than enough :) . I like it's off-loaded to Fab Auth Manager, this is FAB specific and having separate cleanup command looks like a good idea.

Thanks for the feedback! Working on it.

vincbeck

Few comments but overall I really like it better that way! Thanks for the changes!

providers/fab/src/airflow/providers/fab/auth_manager/dag_permissions.py

pierrejeambrun

Looks good overall, a few suggestions.

providers/fab/src/airflow/providers/fab/auth_manager/cli_commands/permissions_command.py

providers/fab/tests/unit/fab/auth_manager/cli_commands/test_permissions_command.py

providers/fab/tests/unit/fab/auth_manager/test_dag_permissions.py

HsiuChuanHsu · 2025-09-03T23:15:09Z

Still working on the testing part, turned the PR into a draft.

This commit adds cleanup logic to the delete_dag function to: - Remove DAG-specific resources from ab_view_menu table - Clean up associated permissions and role associations

- Replace hardcoded resource strings with constants in permissions_command.py - Use RESOURCE_DAG_PREFIX and RESOURCE_DETAILS_MAP for DAG Run - Remove deprecated Task Instance resource handling - Change to session handling with @provide_session decorator - Replace manual boolean parsing with airflow.utils.strings.to_boolean - Consolidate dag_permissions.py functionality into permissions_command.py - Consolidate test files for better maintainability

…al database operations 1. Function call verification (TestPermissionsCommand) - Replace stdout-only testing with precise function call verification - Add mock assertions for cleanup_dag_permissions with exact parameters 2. Real database operations (TestDagPermissions): - Replace mock-heavy approach with actual database interactions - Use real Resource, Action, and Permission entities with constraint handling

HsiuChuanHsu · 2025-09-05T01:35:37Z

Thanks for all the suggestion! This PR is ready for another review.

vincbeck

Nice!

pierrejeambrun

Nice thanks

HsiuChuanHsu · 2025-09-05T14:20:19Z

Thanks for the review! 🤩

* Fix: Clean up FAB permissions when deleting DAGs (apache#50905) This commit adds cleanup logic to the delete_dag function to: - Remove DAG-specific resources from ab_view_menu table - Clean up associated permissions and role associations * test: Add comprehensive tests for DAG permission cleanup Test Coverage: - Non-FAB auth manager scenarios (graceful skip) - FAB provider unavailable scenarios (import error handling) - Full cleanup process with mocked FAB models - Integration with delete_dag function - Edge cases (non-existent DAGs) * fix: clean up DAG permissions when deleting DAGs - : Simplified core logic using auth manager delegation - : Added cleanup_dag_permissions method - : New specialized module for FAB permission cleanup - Enhanced test coverage for multiple auth manager scenarios * feat(fab-auth-manager): replace automatic DAG permission cleanup with CLI command Replace automatic cleanup of DAG permissions during DAG deletion with a new CLI command approach as requested in code review. This gives operators explicit control over when and how DAG permissions are cleaned up. * feat(fab): Update DAG permissions cleanup to SQLAlchemy 2.0 and improve tests - Replace deprecated session.query() with session.scalars(select()) and session.execute(delete()) - Update permissions_command.py to use SQLAlchemy 2.0 syntax for better future compatibility - Rewrite test_permissions_command.py to use integration tests instead of fragile mocks * feat(fab): add missing test_dag_permissions.py Add unit tests for dag_permissions module to satisfy project structure requirements * refactor(fab): Consolidate DAG permissions code and modernize patterns - Replace hardcoded resource strings with constants in permissions_command.py - Use RESOURCE_DAG_PREFIX and RESOURCE_DETAILS_MAP for DAG Run - Remove deprecated Task Instance resource handling - Change to session handling with @provide_session decorator - Replace manual boolean parsing with airflow.utils.strings.to_boolean - Consolidate dag_permissions.py functionality into permissions_command.py - Consolidate test files for better maintainability * test: enhance FAB permissions tests with function verification and real database operations 1. Function call verification (TestPermissionsCommand) - Replace stdout-only testing with precise function call verification - Add mock assertions for cleanup_dag_permissions with exact parameters 2. Real database operations (TestDagPermissions): - Replace mock-heavy approach with actual database interactions - Use real Resource, Action, and Permission entities with constraint handling

HsiuChuanHsu requested review from ephraimbuddy, jason810496, pierrejeambrun and rawwar as code owners August 14, 2025 23:01

boring-cyborg bot added the area:API Airflow's REST/HTTP API label Aug 14, 2025

HsiuChuanHsu force-pushed the bug/remove-deleted-dag-correctly branch from 4da40eb to cda491d Compare August 14, 2025 23:01

HsiuChuanHsu force-pushed the bug/remove-deleted-dag-correctly branch from cda491d to ccbc790 Compare August 19, 2025 23:20

HsiuChuanHsu force-pushed the bug/remove-deleted-dag-correctly branch from ccbc790 to a533d68 Compare August 20, 2025 13:15

pierrejeambrun reviewed Aug 20, 2025

View reviewed changes

airflow-core/src/airflow/api/common/delete_dag.py Outdated Show resolved Hide resolved

HsiuChuanHsu force-pushed the bug/remove-deleted-dag-correctly branch from a533d68 to 51bbff5 Compare August 21, 2025 23:07

HsiuChuanHsu requested a review from vincbeck as a code owner August 21, 2025 23:07

HsiuChuanHsu force-pushed the bug/remove-deleted-dag-correctly branch from 51bbff5 to 2e6c791 Compare August 21, 2025 23:08

vincbeck reviewed Aug 25, 2025

View reviewed changes

airflow-core/tests/unit/api/common/test_delete_dag.py Outdated Show resolved Hide resolved

uranusjr reviewed Aug 27, 2025

View reviewed changes

airflow-core/src/airflow/api/common/delete_dag.py Outdated Show resolved Hide resolved

uranusjr reviewed Aug 27, 2025

View reviewed changes

airflow-core/src/airflow/api/common/delete_dag.py Outdated Show resolved Hide resolved

HsiuChuanHsu force-pushed the bug/remove-deleted-dag-correctly branch 4 times, most recently from f94a416 to e939a76 Compare September 1, 2025 12:39

vincbeck reviewed Sep 2, 2025

View reviewed changes

providers/fab/src/airflow/providers/fab/auth_manager/dag_permissions.py Outdated Show resolved Hide resolved

providers/fab/src/airflow/providers/fab/auth_manager/dag_permissions.py Outdated Show resolved Hide resolved

pierrejeambrun reviewed Sep 2, 2025

View reviewed changes

HsiuChuanHsu marked this pull request as draft September 3, 2025 23:14

HsiuChuanHsu force-pushed the bug/remove-deleted-dag-correctly branch from e939a76 to 0159fd3 Compare September 3, 2025 23:24

Fix: Clean up FAB permissions when deleting DAGs (apache#50905)

de93e08

This commit adds cleanup logic to the delete_dag function to: - Remove DAG-specific resources from ab_view_menu table - Clean up associated permissions and role associations

HsiuChuanHsu added 2 commits September 5, 2025 07:46

HsiuChuanHsu force-pushed the bug/remove-deleted-dag-correctly branch from ae03312 to 44bd723 Compare September 4, 2025 23:46

HsiuChuanHsu marked this pull request as ready for review September 5, 2025 01:34

vincbeck approved these changes Sep 5, 2025

View reviewed changes

pierrejeambrun approved these changes Sep 5, 2025

View reviewed changes

vincbeck merged commit 4d1d318 into apache:main Sep 5, 2025
138 checks passed

HsiuChuanHsu deleted the bug/remove-deleted-dag-correctly branch September 9, 2025 22:19

eladkal mentioned this pull request Sep 18, 2025

Status of testing Providers that were prepared on September 18, 2025 #55831

Closed

78 tasks

	def clear_dag_specific_permissions():
	if "FabAuthManager" not in conf.get("core", "auth_manager"):
	return
	try:
	from airflow.providers.fab.auth_manager.models import Permission, Resource, assoc_permission_role
	except ImportError:
	# Handle Pre-airflow 2.9 case where FAB was part of the core airflow
	from airflow.providers.fab.auth.managers.fab.models import (
	Permission,
	Resource,
	assoc_permission_role,
	)
	except RuntimeError as e:
	# Handle case where FAB provider is not even usable
	if "needs Apache Airflow 2.9.0" in str(e):
	from airflow.providers.fab.auth.managers.fab.models import (
	Permission,
	Resource,
	assoc_permission_role,
	)
	else:
	raise
	with create_session() as session:
	dag_resources = session.query(Resource).filter(Resource.name.like(f"{RESOURCE_DAG_PREFIX}%")).all()
	dag_resource_ids = [d.id for d in dag_resources]

	dag_permissions = session.query(Permission).filter(Permission.resource_id.in_(dag_resource_ids)).all()
	dag_permission_ids = [d.id for d in dag_permissions]

	session.query(assoc_permission_role).filter(
	assoc_permission_role.c.permission_view_id.in_(dag_permission_ids)
	).delete(synchronize_session=False)
	session.query(Permission).filter(Permission.resource_id.in_(dag_resource_ids)).delete(
	synchronize_session=False
	)
	session.query(Resource).filter(Resource.id.in_(dag_resource_ids)).delete(synchronize_session=False)

Fix: Clean up FAB permissions when deleting DAGs #54528

Fix: Clean up FAB permissions when deleting DAGs #54528

Uh oh!

Conversation

HsiuChuanHsu commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How this happend?

Solution

Replace automatic DAG permission cleanup with CLI command

Uh oh!

potiuk commented Aug 18, 2025

Uh oh!

HsiuChuanHsu commented Aug 19, 2025

Uh oh!

pierrejeambrun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincbeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HsiuChuanHsu commented Aug 26, 2025

Uh oh!

potiuk commented Aug 27, 2025

Uh oh!

Uh oh!

Uh oh!

HsiuChuanHsu commented Aug 28, 2025

Uh oh!

vincbeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pierrejeambrun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HsiuChuanHsu commented Sep 3, 2025

Uh oh!

HsiuChuanHsu commented Sep 5, 2025

Uh oh!

vincbeck left a comment

Choose a reason for hiding this comment

Uh oh!

pierrejeambrun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HsiuChuanHsu commented Sep 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HsiuChuanHsu commented Aug 14, 2025 •

edited

Loading