Skip to content

Standardize default fab perms#14946

Merged
jhtimmins merged 16 commits into
apache:masterfrom
astronomer:standardize-default-fab-perms
Apr 12, 2021
Merged

Standardize default fab perms#14946
jhtimmins merged 16 commits into
apache:masterfrom
astronomer:standardize-default-fab-perms

Conversation

@jhtimmins

@jhtimmins jhtimmins commented Mar 23, 2021

Copy link
Copy Markdown
Contributor

This PR applies the Stable API permissions model to the existing FAB permission views.

In order to merge, the new /users and /roles endpoints were updated to use the FAB permissions model.

The following mapping changes are included

PermissionModelView.can_list => Permissions.can_read
PermissionViewModelView.can_list => Permission Views.can_read
ResetMyPasswordView.can_this_form_get => My Password.can_read
ResetMyPasswordView.can_this_form_post => My Password.can_edit
ResetPasswordView.can_this_form_get => Passwords.can_read
ResetPasswordView.can_this_form_post => Passwords.can_edit
RoleModelView.can_delete => Roles.can_delete
RoleModelView.can_download => Roles.can_read
RoleModelView.can_show => Roles.can_read
RoleModelView.can_list => Roles.can_read
RoleModelView.can_edit => Roles.can_edit
RoleModelView.can_add => Roles.can_create
RoleModelView.can_copyrole => Roles.can_create
ViewMenuModelView.can_list => View Menus.can_read
UserDBModelView.can_add => View Menus.can_create
UserDBModelView.can_userinfo => My Profile.can_read
UserDBModelView.can_download => View Menus.can_read
UserDBModelView.can_show => View Menus.can_read
UserDBModelView.can_list => View Menus.can_read
UserDBModelView.can_edit => View Menus.can_edit
UserDBModelView.resetmypassword => My Password.can_read
UserDBModelView.resetpasswords => Passwords.can_read
UserDBModelView.userinfoedit => My Profile.can_edit
UserDBModelView.can_delete => View Menus.can_delete
UserInfoEditView.can_this_form_get => My Profile.can_read
UserInfoEditView.can_this_form_post => My Profile.can_edit
UserStatsChartView.can_chart => User Stats Chart.can_read
UserLDAPModelView.can_userinfo => My Profile.can_read
UserOAuthModelView.can_userinfo => My Profile.can_read
UserOIDModelView.can_userinfo => My Profile.can_read
UserRemoteUserModelView.can_userinfo => My Profile.can_read
DagRunModelView.can_clear => Dags.can_read, TaskInstances.can_delete

@boring-cyborg boring-cyborg Bot added area:API Airflow's REST/HTTP API area:webserver Webserver related Issues kind:documentation labels Mar 23, 2021
@jhtimmins jhtimmins force-pushed the standardize-default-fab-perms branch 2 times, most recently from bb2378d to 71f4062 Compare March 24, 2021 01:51
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@jhtimmins jhtimmins force-pushed the standardize-default-fab-perms branch from 71f4062 to 7f3d0c0 Compare March 24, 2021 22:11
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if there's a mistake here on using RESOURCE_PERMISSION_VIEW?

@ephraimbuddy ephraimbuddy Mar 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah...Got it

@kaxil kaxil force-pushed the standardize-default-fab-perms branch from 7f3d0c0 to 434928e Compare March 25, 2021 15:49
@kaxil

kaxil commented Mar 25, 2021

Copy link
Copy Markdown
Member

Rebased to latest master to fix the failing errors around providers

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also set conf.getboolean('webserver', 'UPDATE_FAB_PERMS'): somehow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashb What would be the purpose of that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly couldn't tell you anymore.

kaxil pushed a commit that referenced this pull request Mar 31, 2021
Converts the docs_link permission resource name to Documentation.

This is an extension of #14946, which standardized default FAB permissions.
@jhtimmins jhtimmins force-pushed the standardize-default-fab-perms branch from 434928e to 3bed2c2 Compare March 31, 2021 18:41
@ashb

ashb commented Mar 31, 2021

Copy link
Copy Markdown
Member

FYI: This PR and #14840 "fight"

@jhtimmins

jhtimmins commented Mar 31, 2021

Copy link
Copy Markdown
Contributor Author

@ashb This updates the permission model, so #14840 will need to integrate these changes, which I'm happy to help with.
Can you and @kaxil take a look?

@jhtimmins jhtimmins requested review from ashb and ephraimbuddy April 2, 2021 23:01
@kaxil kaxil added this to the Airflow 2.1 milestone Apr 2, 2021
Comment thread airflow/www/views.py Outdated
Comment on lines 3913 to 3945

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between this two? Just want to make sure we don't accidentally give permissions to a user to change password for other users :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resetmypassword just lets a user update their own password.

resetpasswords lets them update any user's password, and should be limited to Admin-level users.

Comment thread airflow/www/views.py Outdated
Comment on lines 3972 to 4014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason we call "/userstatschartview" as RESOURCE_USER_STATS_CHART but for userinfo one we call it RESOURCE_MY_PROFILE

i.e. RESOURCE_MY vs RESOURCE_USER. Should we standardize it to say RESOURCE_USER_STATS_CHART and the other one as RESOURCE_USER_PROFILE?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

userstatschartview applies to all users, whereas RESOURCE_MY_PROFILE only applies to the logged in user.

@kaxil kaxil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments otherwise LGTM

@jhtimmins jhtimmins force-pushed the standardize-default-fab-perms branch from 1822c42 to 4b8ec3b Compare April 12, 2021 18:01
@jhtimmins jhtimmins force-pushed the standardize-default-fab-perms branch from 4b8ec3b to 20e6b9a Compare April 12, 2021 18:01
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions Bot added the full tests needed We need to run full set of tests for this PR to merge label Apr 12, 2021
@jhtimmins jhtimmins merged commit 18c5b8a into apache:master Apr 12, 2021
@jhtimmins jhtimmins deleted the standardize-default-fab-perms branch April 12, 2021 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API area:webserver Webserver related Issues full tests needed We need to run full set of tests for this PR to merge kind:documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants