Skip to content

Allow all default roles to view Profile page + allow editing profile/resetting password if it's DB-ModelView#12971

Merged
XD-DENG merged 1 commit intoapache:masterfrom
XD-DENG:allow-userinfo-and-resetmypassword
Dec 10, 2020
Merged

Allow all default roles to view Profile page + allow editing profile/resetting password if it's DB-ModelView#12971
XD-DENG merged 1 commit intoapache:masterfrom
XD-DENG:allow-userinfo-and-resetmypassword

Conversation

@XD-DENG
Copy link
Member

@XD-DENG XD-DENG commented Dec 9, 2020

This is a change discussed long time back in #3889 (comment)

Currently, ONLY Admin role is allowed to view "Profile" page, and edit profile info or reset password there. All other roles will get "Access Denied" if they click this Your Profile button.

Airflow
Airflow-2

The ideal situation should be:

  • all (default) roles should be able to view their profile page.
  • all (default) roles should be able to edit their profile (change first/last names), only if the webserver is DB-based.
  • all (default) roles should be able to reset their password, only if the webserver is DB-based.
    (For example, if the webserver is using LDAP or OAuth, users should NOT be allowed to edit their profile or reset password.)

Essentially, the 7 permission-resource pairs are added for all default roles:

  • can_this_form_post on UserInfoEditView
  • can_this_form_get on UserInfoEditView
  • can_userinfo on UserDBModelView
  • userinfoedit on UserDBModelView
  • can_this_form_post on ResetMyPasswordView
  • can_this_form_get on ResetMyPasswordView
  • resetmypassword on UserDBModelView

In addition, can_userinfo are added for all other possible User ModelViews (UserOIDModelView, UserLDAPModelView, UserOAuthModelView, and UserRemoteUserModelView. Reference)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@XD-DENG XD-DENG added area:webserver Webserver related Issues type:improvement Changelog: Improvements labels Dec 9, 2020
@XD-DENG XD-DENG requested review from ashb, kaxil and turbaszek December 9, 2020 20:44
@XD-DENG
Copy link
Member Author

XD-DENG commented Dec 9, 2020

Hi @ashb @kaxil @turbaszek , this PR is to implement what has been discussed long time back in #3889 (comment) . @ashb has done very thorough analysis in that discussion, and this PR is simply following what has been discussed there.

Understand now the focus in on 2.0.0rc1. Take your time if you have no bandwidth on this, and I can ping again only later.

@github-actions
Copy link

github-actions bot commented Dec 9, 2020

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

@ashb ashb added this to the Airflow 2.1 milestone Dec 10, 2020
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

I thought I made this change aaages go, clearly not :D

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 10, 2020
@github-actions
Copy link

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.

…resetting password if it's DB-ModelView.

This is a change discussed long time back in apache#3889 (comment)

Essentially, the 7 permission-resource pairs are added for all users:
- can_this_form_post on UserInfoEditView
- can_this_form_get on UserInfoEditView
- can_userinfo on UserDBModelView
- userinfoedit on UserDBModelView
- can_this_form_post on ResetMyPasswordView
- can_this_form_get on ResetMyPasswordView
- resetmypassword on UserDBModelView

In addition, can_userinfo is added for all possible User ModelViews, so users can also view profile when
the webserver is using different setting-ups.
But they are ONLY allowed to edit profile and reset password when it's UserDBModelView
@XD-DENG XD-DENG force-pushed the allow-userinfo-and-resetmypassword branch from 9a5e35d to 5559920 Compare December 10, 2020 12:46
@XD-DENG
Copy link
Member Author

XD-DENG commented Dec 10, 2020

I thought I made this change aaages go, clearly not :D

Yes, something from long time ago 😄 I happened to revisit this. Credit to you for the very detailed analysis you have done.

Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

@XD-DENG XD-DENG merged commit fbd8348 into apache:master Dec 10, 2020
@jhtimmins
Copy link
Contributor

@XD-DENG @kaxil For the new resource-based permissions, we're actively trying to create resources that reflect the underlying data, rather than a specific view. In terms of actions, we're trying to stick to Read, Edit, Create, and Delete. This will allow permissions in the UI to be used in the API as well if we choose to include all UI functionality in the API at some point.

We can either revert this PR or create another PR to get these permissions in line with the existing approach.

@ashb
Copy link
Member

ashb commented Dec 10, 2020

@jhtimmins These are views/perms from FAB-managed views, so I'm not sure we can do much about them (at least not easily?)

@XD-DENG
Copy link
Member Author

XD-DENG commented Dec 10, 2020

As @ashb mentioned, all these are the "built-in" stuff provided by FAB, and actually they have almost zero linkage with Airflow itself.

So personally I don't think it's making sense to "re-create" anything on top of them.

@XD-DENG XD-DENG deleted the allow-userinfo-and-resetmypassword branch December 10, 2020 16:16
@XD-DENG XD-DENG modified the milestones: Airflow 2.1, Airflow 2.0rc2 Dec 12, 2020
@XD-DENG
Copy link
Member Author

XD-DENG commented Dec 12, 2020

Updating milestone to 2.0rc2 since I notice this has been cherry-picked into the latest 2.0.0rc2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues full tests needed We need to run full set of tests for this PR to merge type:improvement Changelog: Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants