huymq1710 commented on code in PR #30116:
URL: https://github.com/apache/airflow/pull/30116#discussion_r1145682488
##########
airflow/www/security.py:
##########
@@ -662,8 +661,17 @@ def _revoke_stale_permissions(resource: Resource):
for perm in existing_dag_perms:
non_admin_roles = [role for role in perm.role if role.name !=
"Admin"]
for role in non_admin_roles:
- target_perms_for_role = access_control.get(role.name, {})
- if perm.action.name not in target_perms_for_role:
+ if access_control:
+ target_perms_for_role = access_control.get(role.name,
{})
+ if perm.action.name not in target_perms_for_role:
+ self.log.info(
+ "Revoking '%s' on DAG '%s' for role '%s'",
+ perm.action,
+ dag_resource_name,
+ role.name,
+ )
+ self.remove_permission_from_role(role, perm)
+ else:
Review Comment:
Hi @jedcunningham,
In my
[66e7cb9](https://github.com/apache/airflow/pull/30116/commits/66e7cb91938c52deb883e0ca4dd2f8d011e867a8),
if dag_was_updated, it will call
[sync_perm_for_dag()](https://github.com/apache/airflow/pull/30116/files#diff-a2cc1cc05747aab5c8f90ce72a599322d32f69fbfa1e9474655f0471d500e2dfR691)
for all case. And check whether there’s existing permission configurations and
reset those if `access_control` is empty.
However, `test_views_home.py` failed:
```
FAILED
tests/www/views/test_views_home.py::test_home_importerrors_filtered_singledag_user[home]
- AssertionError: Couldn't find 'Import Errors'
FAILED
tests/www/views/test_views_home.py::test_home_importerrors_filtered_singledag_user[home?status=active]
- AssertionError: Couldn't find 'Import Errors'
FAILED
tests/www/views/test_views_home.py::test_home_importerrors_filtered_singledag_user[home?status=paused]
- AssertionError: Couldn't find 'Import Errors'
FAILED
tests/www/views/test_views_home.py::test_home_importerrors_filtered_singledag_user[home?status=all]
- AssertionError: Couldn't find 'Import Errors'
FAILED
tests/www/views/test_views_home.py::test_home_dag_list_filtered_singledag_user
- AssertionError: Couldn't find 'dag_id=filter_test_1'
FAILED tests/www/views/test_views_home.py::test_home_dag_edit_permissions -
AssertionError: assert ('filter_test_1', True) in [('a_first_dag_id_asc',
False), ('filter.test', False), ('filter_test_1', False), ('filter_test_2',
False)]
```
I believe my above logic also affects
https://github.com/apache/airflow/blob/main/tests/www/views/test_views_home.py#L88-L99
Is my logic right ? What should I do in next step ?
Hope to hear your advice. 🙇
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
