potiuk commented on issue #10469: URL: https://github.com/apache/airflow/issues/10469#issuecomment-679013603
Agree it's confusing. Initially we wanted to redefine the model of access completely with the new API but I think we decided finally that we accept some imperfections of the FAB model - because we already had it and we already have Airflow Users who - at least in theory know how to change them. And indeed with 2.0 it is the right time to have a "spring cleaning" even if it means changing some existing roles. It is important though that we do it in the way that we can migrate the old permissions to the new ones automatically on migration. Which should be possible IMHO (with some caveats and edge-cases of course, but should be). Tying the permissions to API resources makes much more sense than having then tied to the UI views. But this seems like quite an effort and I think we can discuss it today at 2.0 whether it's necessary/needed/possible to get it in 2.0. I believe we should do it for 2.0. > * **Variants of create /read/edit/delete for Airflow modelViews** - (ex. `domain: RoleModelView, permission: can_show`) - Consolidate into `can_create`, `can_read`, `can_edit`, and `can_delete` for the `Role` resource. Agree. > * **Menu access for Airflow modelViews** - (ex. `domain: Task Instances, permission: menu_access`) - We should do away with menu-specific permissions. If a user has read access for the resource, they should have menu access in the UI. Agree with that as well, however, the question is how we maintain links between Menus and permissions. It should be possible to annotate each menu/view with the permissions needed. And check if the user has the right permissions at the time we display view. Another (not such nice IMHO) method will be to keep the permissions still but somehow manage the assignment of the permissions automatically - but that might be a nightmare to do. > * **Muldelete for Airflow modelViews** - (ex. `domain: PoolModelView, permission: muldelete`) - I propose combining `muldelete` and `delete` into a single `delete` action. If you can delete one, you can delete multiple. Agree. No reason to separate muldelete from delete. Multiple delete shoudl simply be always confirmed with "are you sure you want to delete those 100 DAGs ???". > * **List for Airflow modelViews** - (ex. `domain: PoolModelView, permission: can_list`) - Same as `muldelete`. I propose combining `list` and `read` into a single `read` permission. Agree. It's Far too fine-grained I think. I see no point in being able to see the list of DAGs but do not see how many times it executed. > * **Default FAB permissions** - (ex. `domain: ResetPasswordView, permission: can_this_form_post`) - I propose leaving these in place. We technically could change these to match the new pattern by subclassing the default FAB views and settings custom permission mappings. If we choose to do that, it should be part of a separate issue. Agree. > * **Edit/read for DAGs** - (ex. `domain: example_branch_dop_operator_v3, permission: can_dag_read`) - Change to `can_edit` and `can_read` for the same `example_branch_dop_operator_v3` domain. Yes. If we standardize the permission names we should do it also for that. > * **Airflow view permissions** - The following is a list of examples, mapping existing permissions to proposed new ones. Where possible, this involves mapping the view permission to `model.can_read`, or one of the other CRUD actions. Agree. Mapping it to resources makes perfect sense. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
