[GitHub] [airflow] mik-laj commented on issue #10469: Make permissions consistent between UI and API

[GitBox <gi...@apache.org>]

mik-laj commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-679063320


   > 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.
   
   I love it <3 
   
   > 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.
   
   I agree, but it must be checked carefully if it will be possible to do in the FAB. I would like to see it also as a contribution to the FAB, so that it would be possible to personalize the name of this permission.
   
   It will probably look like below.
   ```diff
        def get_user_menu_access(self, menu_names: List[str] = None) -> Set[str]:
            if current_user.is_authenticated:
                return self._get_user_permission_view_menus(
   -                g.user, "menu_access", view_menus_name=menu_names
   +                g.user, self.menu_access.permisssion, view_menus_name=menu_names
                )
            elif current_user_jwt:
                return self._get_user_permission_view_menus(
   -                current_user_jwt, "menu_access", view_menus_name=menu_names
   +                current_user_jwt, self.menu_access.permisssion, view_menus_name=menu_names
                )
            else:
                return self._get_user_permission_view_menus(
   -                None, "menu_access", view_menus_name=menu_names
   +                None, self.menu_access.permisssion, view_menus_name=menu_names
                )
   ```
   
   > 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.
   
   > 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. This will simplify the management of permissions.
   
   > 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. We should limit the introduced changes only to the most important ones.
   
   > 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.
   
   This can lead to name collisions and I think it is worth making sure that a DAG that has an ID = pool is easy to distinguish from the rest of the permissions.
   
   > Airflow.can_refresh_all => Dag.can_read
   
   This is the operation that modifies the data and is sent as POST. Rather, it should be can_edit.
   
   > Airflow.can_index => Seems unnecessary?
   
   It's a DAG list, so DAG.can_read
   
   > Airflow.can_blocked => Dag.can_read (does this modify anything?)
   
   Can you tell a little more about it?
   


----------------------------------------------------------------
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:
users@infra.apache.org