You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/22 05:24:48 UTC
[GitHub] [airflow] jhtimmins opened a new issue #10469: Make permissions consistent between UI and API
jhtimmins opened a new issue #10469:
URL: https://github.com/apache/airflow/issues/10469
## Rationale
Airflow currently has 185 permissions (where 1 permission is a combo of Class + Action). The design of the permissions is inconsistent, making them difficult to modify or organize into roles. This is especially problematic for the stable API. Because the UI permissions are named after the UI view and methods, and are intended to control access to the UI view rather than the underlying resource, they don't map cleanly to API methods.
We need to define a set of permissions that control access to underlying resources, independent from the interface used to access those resources.
### Aside about names
Flask AppBuilder names permissions in a confusing manner. It uses the term "permission" to refer to both an action (for example, "can_edit") and an action + resource pair ("can_edit on the TaskInstanceModelView"). This document refers to a `permission` as a unique combination of `action` + `domain`, where `domain` is the entity to which access is granted. For old permission design, the domain is the UI view class. For new permission design, the domain refers to the resource model.
**Description**
There are multiple types of permissions. We should consider each in turn.
* **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.
* **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.
* **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.
* **List for Airflow modelViews** - (ex. `domain: PoolModelView, permission: can_list`) - Same as `muldelete`. I propose combining `list` and `read` into a single `read` permission.
* **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.
* **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.
* **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.
> * Airflow.can_redirect_to_external_log => TaskInstance.can_read
> * Airflow.can_delete => Dag.can_delete
> * Airflow.can_task => Task.can_read
> * Airflow.can_trigger => Dag.can_trigger
> * Airflow.can_dag_details => Dag.can_read
> * Airflow.can_clear => TaskInstance.can_delete
> * Airflow.can_refresh_all => Dag.can_read
> * Airflow.can_extra_links => Dag.can_edit
> * Airflow.can_index => Seems unnecessary?
> * Airflow.can_refresh => Dag.can_read
> * Airflow.can_xcom => XCom.can_read
> * Airflow.can_rendered => TaskInstance.can_read
> * Airflow.can_blocked => Dag.can_read (does this modify anything?)
**Additional considerations**
There are two motivations.
1. Make view permissions fit for API endpoints.
2. Simplify permissions to make them more usable.
Ultimately, there are two key questions for this issue:
1. Are these permission updates the right changes to make.
2. If they're the right ones, is now the right time to make them?
**Related Issues**
#8112
----------------------------------------------------------------
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
[GitHub] [airflow] ashb commented on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-686422184
> Airflow.can_extra_links => Dag.can_edit
That should be `Dag.can_read` I think.
> Airflow.can_index
The one possible advantage of this is it's useful in some cases - for instance https://github.com/apache/airflow/pull/4973 where the they wanted public user to have access to the home page, but no DAGs.
> Airflow.can_refresh => Dag.can_read
Do we even need the refresh button anymore with DAG serialization? (Are we making that default/only option for 2.0?)
> * **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.
What specifically are we calling the permssion and view menu for dag-level permissions?
`"can_read" on "dag.example_branch_dop_operator_v3" (i.e. view menu name becomes `"dag.example_branch_dop_operator_v3"`?)
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj commented on issue #10469: Migrate existing views/endpoints to use resource-based permissions
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-743766755
Is is done.
----------------------------------------------------------------
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
[GitHub] [airflow] ashb commented on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-687279853
> Airflow.can_trigger => Dag.can_trigger
This should be `DagRun.can_create` to match https://github.com/apache/airflow/pull/10594/files#
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj commented on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-680351929
@jhtimmins Can I help with this ticket? If you have a blocker, let me know and we'll try to find a solution together.
----------------------------------------------------------------
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
[GitHub] [airflow] kaxil commented on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-681086447
>Airflow.can_refresh_all => Dag.can_read
Since `/refresh` also needs `can_dag_edit` permissions, I think `refresh_all` should be the same.
But other than that, doc looks good.
----------------------------------------------------------------
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
[GitHub] [airflow] kaxil commented on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-686443915
`Airflow.can_refresh` can be used to force fetch Serialized DAG version from the DB instantly
----------------------------------------------------------------
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
[GitHub] [airflow] jhtimmins commented on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-678597419
@mik-laj @kaxil @ashb @potiuk Would love to hear your thoughts on renaming permissions to make them consistent across the UI and API.
----------------------------------------------------------------
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
[GitHub] [airflow] kaxil edited a comment on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-686443915
`Airflow.can_refresh` can be used to force fetch Serialized DAG version from the DB instantly, there is also `/refresh_all` which will fetch all the Serialized DAGs from DB
----------------------------------------------------------------
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
[GitHub] [airflow] ashb commented on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-686531621
Did I make up that we would remove the need for a refresh button at all -- i.e. we could just look at the last_updated column on s10n_dag row on every web request?
Mostly because the "refresh" button in the webserver never worked very well -- that would only refresh it for that one web-worker, and by default there are 4, so if you get a refreshed version or not depends on which worker serves the request.
----------------------------------------------------------------
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
[GitHub] [airflow] kaxil commented on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #10469:
URL: https://github.com/apache/airflow/issues/10469#issuecomment-686551595
It also sync's permissions though. Clairvoyant were using it: https://github.com/apache/airflow/issues/9749 | https://github.com/teamclairvoyant/airflow-rest-api-plugin/issues/63
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj closed issue #10469: Migrate existing views/endpoints to use resource-based permissions
Posted by GitBox <gi...@apache.org>.
mik-laj closed issue #10469:
URL: https://github.com/apache/airflow/issues/10469
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj commented on issue #10469: Make permissions consistent between UI and API
Posted by 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
[GitHub] [airflow] potiuk commented on issue #10469: Make permissions consistent between UI and API
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org